Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add towncrier #220

Closed
wants to merge 4 commits into from
Closed

Add towncrier #220

wants to merge 4 commits into from

Conversation

mpnowacki-reef
Copy link

No description provided.

@mpnowacki-reef mpnowacki-reef force-pushed the towncrier branch 2 times, most recently from ea398a2 to 9c0d50e Compare November 18, 2023 12:48
Copy link

@mjurbanski-reef mjurbanski-reef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very happy this is happening, thank you :)

CHANGELOG.md Show resolved Hide resolved
@@ -0,0 +1 @@
Towncrier changelog generation - to avoid conflicts when simultaneously working on PRs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go into Infrastracture section not Added section, as this is something that should not affect end users in any case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, I considered this a feature, but it is only one from a perspective of a developer

@@ -1,22 +1,58 @@
# Contributing to B2 Command Line Tool

We encourage outside contributors to perform changes to our codebase. Many such changes have been merged already. In order to make it easier to contribute, core developers of this project:
We encourage outside contributors to perform changes to our codebase. Many such changes have been merged already.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sembr

apparently readable formatter must be broken if it didn't catch lack of linebreak after period here.

It is worth investigating, as it very much may want to change the prefer list element character (*/-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this formatter? is it anywhere in nox/CI?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, we don't have it here; nvm that then

still please put newline after . per sembr rules

noxfile.py Outdated Show resolved Hide resolved
4. deprecated
5. removed
6. infrastructure
7. doc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we shouldn't be inventing new sections if we are commiting to keepachangelog standard... we already added infrastracture, lets not add more. doc instead documentation feels weird as well...

I don't have that strong of a opinion here (i.e. I will be ok with whatever will be your finall call), but just letting you know it doesn't feel right to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite a lot of stuff we do is about docs (like your last PR) I don't see a reason to not add sections we actually use since we have already diverted from keepachangelog

with:
token: ${{ github.token }}
- name: Validate new changelog entries
run: if [ -z "$(git diff --diff-filter=A --name-only origin/${{ github.event.pull_request.base.ref }} changelog.d)" ];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't validate if new section added is an allowed section or not - even while testing I did a typo and this would certainly not catch it - I only realised when a file was leftover after changelog generation - and only after looking changelog.d myself, as towncrier build does not check if changelog.d is empty afterwards...

can't we use https://towncrier.readthedocs.io/en/stable/cli.html#towncrier-check here ?

Copy link
Author

@mpnowacki-reef mpnowacki-reef Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, check doesn't help that much either, unfortunately

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still need - this mistake is too easy to do and even during release we are very unlikely to catch it with

please follow up on this

Copy link
Author

@mpnowacki-reef mpnowacki-reef Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

towncrier check only detects typos in news types if:

  • Changelog is otherwise unedited; and
  • there is only one news item in the PR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not ideal, but still better than never I guess

pyproject.toml Outdated
Comment on lines 31 to 64
[[tool.towncrier.type]]
directory = "infrastructure"
name = "Infrastructure"
showcontent = true

[[tool.towncrier.type]]
directory = "removed"
name = "Removed"
showcontent = true

[[tool.towncrier.type]]
directory = "deprecated"
name = "Deprecated"
showcontent = true

[[tool.towncrier.type]]
directory = "added"
name = "Added"
showcontent = true

[[tool.towncrier.type]]
directory = "changed"
name = "Changed"
showcontent = true

[[tool.towncrier.type]]
directory = "fixed"
name = "Fixed"
showcontent = true

[[tool.towncrier.type]]
directory = "doc"
name = "Doc"
showcontent = true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please reorder these, from the MUST READ to "not that important" to the end user
i.e.
"Infrastructure" for sure should go at very end, and Removed/Changed (i.e. breaking changes) should go 1st and 2nd,

then Fixed, Deprecated, Added - but I don't feel as strongly about those.

README.release.md Show resolved Hide resolved
Comment on lines +53 to +54
`towncrier create` also takes care of duplicates automatically (if there is more than 1 news fragment of one type
for a given github issue).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sounds quite scary - can you lead me to documentation of this behavior?

https://towncrier.readthedocs.io/en/stable/cli.html#towncrier-create suggest new file will be created every time

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're there, in the paragraph you mentioned it says

f the filename exists already, towncrier create will add (and then increment) a number after the fragment type until it finds a filename that does not exist yet. In the above example, it will generate 123.bugfix.1.rst if 123.bugfix.rst already exists.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok, that is what you meant. I though you wanted to say it deduplicates things :) thx for clarifying it for me

with:
token: ${{ github.token }}
- name: Validate new changelog entries
run: if [ -z "$(git diff --diff-filter=A --name-only origin/${{ github.event.pull_request.base.ref }} changelog.d)" ];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not ideal, but still better than never I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants