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

Ignore types on check #618

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Conversation

bentheiii
Copy link
Contributor

@bentheiii bentheiii commented Jun 20, 2024

Description

closes #617
Adds a new optional key to newsfragment categories called check (boolean, defaults to true). If set to false, towncrier check will not consider newsfragments of this category as enough to pass

Checklist

  • Make sure changes are covered by existing or new tests.
  • For at least one Python version, make sure local test run is green.
  • Create a file in src/towncrier/newsfragments/. Describe your
    change and include important information. Your change will be included in the public release notes.
  • Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
  • Ensure docs/tutorial.rst is still up-to-date.
  • If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
  • If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

@bentheiii bentheiii requested a review from a team as a code owner June 20, 2024 10:57
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Many thanks for the PR.

It looks good.

I left a few minor comments

Major comments:

  • The only_check_categories is confusing... especially without any extra documentation... but I hope we can have it removed.
  • The tests needs a docstring/description

Thanks again

src/towncrier/test/test_check.py Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
src/towncrier/_builder.py Outdated Show resolved Hide resolved
src/towncrier/test/test_check.py Outdated Show resolved Hide resolved
@bentheiii bentheiii requested a review from adiroiban June 20, 2024 13:49
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks.

Only minor comments.

I will let this PR unmerged for a few days to see if @SmileyChris has any extra feedback about this PR.

No news is good news, is we can check on Monday and see if this can be merged.

Thanks again

@@ -221,6 +221,11 @@ These may include the following optional keys:
Orphan fragments (those without an issue number) always have their content included.
If a fragment was created, it means that information is important for end users.

``check``
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for not raising this in the first place. My bad.

Should this be more explicit / specific ?

Mabye in the future we would want to add some other things related to check behaviour. ... I am not sure what we might add... so this is just a minor comment.

Suggested change
``check``
``ignore_check``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that if this were ever needed, it would be easier to make the check a dict|bool rather than a separete knob from the ignore_check

[[tool.towncrier.type]]
...
check=false
[[tool.towncrier.type]]
...
check=true
[[tool.towncrier.type]]
...
check={"some_new_flag": "some_value"}

Copy link
Member

Choose a reason for hiding this comment

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

check={"some_new_flag": "some_value"}

this looks bad to me... if we use this, why bother with .INI or .TOML and not just go with JSON ?

If you dislike ignore_check then we can keep the current option.

@@ -221,6 +221,11 @@ These may include the following optional keys:
Orphan fragments (those without an issue number) always have their content included.
If a fragment was created, it means that information is important for end users.

``check``
A boolean value indicating whether the fragment should be considered by the ``towncrier check`` command.
Copy link
Member

Choose a reason for hiding this comment

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

This is not very clear... but I don't know how to better explain ths... writing good documentation is hard :)

Maybe

Suggested change
A boolean value indicating whether the fragment should be considered by the ``towncrier check`` command.
A boolean value indicating whether the fragment should be considered by the ``towncrier check`` command.
When set to `false`, ``towncrier check`` will fail, unless the branch contains other newsfragment types.

@bentheiii
Copy link
Contributor Author

bentheiii commented Jun 26, 2024

Hi @adiroiban, can we merge this?

@adiroiban adiroiban merged commit b49cca6 into twisted:trunk Jun 26, 2024
16 checks passed
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.

allow specifying fragment types to consider as "valid" for towncrier check
2 participants