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

Check if any news fragments have invalid filenames #622

Merged
merged 8 commits into from
Jul 29, 2024

Conversation

MetRonnie
Copy link
Contributor

@MetRonnie MetRonnie commented Jul 10, 2024

Description

Closes #619
Fixes #426

towncrier check will now fail if any news fragments have invalid filenames.

Added a new configuration option called build_ignore_filenames that allows you to specify a list of filenames that should be ignored. If this is set, towncrier build will also fail if any filenames are invalid, except for those in the list.

Added a --strict option to towncrier build to enable this behaviour for when build_ignore_filenames has not been configured.

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. (Not applicable)

src/towncrier/_builder.py Outdated Show resolved Hide resolved
src/towncrier/build.py Outdated Show resolved Hide resolved
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.

Thanks for working on this.

This needs documentation and tests to be merged.


While reviewing this, my question is.

Why do we ever want to allow towncrier build or towncrier check to succeed if there are in invalid fragments.

I understand that you might want to have other files inside the newsfragment folder (for example the template and a README) but other than that, I think towncrier should fail

Cheers

@MetRonnie MetRonnie changed the title Add --check-names option to towncrier build Check if any news fragments have invalid filenames Jul 12, 2024
@MetRonnie MetRonnie marked this pull request as ready for review July 12, 2024 11:11
@MetRonnie MetRonnie requested a review from a team as a code owner July 12, 2024 11:11
@MetRonnie
Copy link
Contributor Author

MetRonnie commented Jul 12, 2024

I think all feedback has been addressed now. Let me know if there is anything else.

Maybe a warning if towncrier build encounters invalid filenames but build_ignore_filenames has not been configured? Do you envisage deprecating the --strict option and making the fail behaviour the default in a future version of Towncrier?

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 changes.

They look good.

As a general rule, I think that is best to start any new functionality by writing the documentation.

In this way, we can agree on how the feature should behave, before writing more code.


I think that we can have this implemented without the --strict option.

In terms for testing I am expecting 3 main test scenarios:

  • build_ignore_filenames not set
  • build_ignore_filenames is set to empty list
  • build_ignore_filenames is set with some filenames

Thanks again for working on this!

docs/cli.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/tutorial.rst Outdated Show resolved Hide resolved
docs/tutorial.rst Outdated Show resolved Hide resolved
src/towncrier/_builder.py Outdated Show resolved Hide resolved
src/towncrier/test/test_build.py Outdated Show resolved Hide resolved
src/towncrier/test/test_build.py Outdated Show resolved Hide resolved
src/towncrier/test/test_build.py Outdated Show resolved Hide resolved
src/towncrier/test/test_check.py Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
@adiroiban adiroiban mentioned this pull request Jul 13, 2024
@adiroiban
Copy link
Member

adiroiban commented Jul 15, 2024

I forgot to leave this feedback

Maybe a warning if towncrier build encounters invalid filenames but build_ignore_filenames has not been configured?

That would be nice. We can also have this in a separate PR.
This should not be a blocker.


Do you envisage deprecating the --strict option and making the fail behaviour the default in a future version of Towncrier?

I hope that we don't have to add the --strict option... so that we don't have to deprecate it.

It would be nice to have a warinig raised when build_ignore_filenames is not configured and make a release that raised the warning.

In a followup release, we can then change the default value of build_ignore_filenames so thta it will alwasy do a strict check by default... while still allowing users to configure it with build_ignore_filenames = None`

@adiroiban
Copy link
Member

I have updated the PR description to note that it will also fix #426 ... the #619 might be a duplicate

- Change `build_ignore_filenames` to `ignore` in config
- Remove `--strict` option from `towncrier build`
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 changes.

They look good.

I have to still review the automated tests... but the other part looks good.

src/towncrier/_builder.py Outdated Show resolved Hide resolved
src/towncrier/_builder.py Outdated Show resolved Hide resolved
@@ -101,12 +101,14 @@ def __main(
click.echo(f"{n}. {change}")
click.echo("----")

# This will fail if any fragment files have an invalid name:
Copy link
Member

Choose a reason for hiding this comment

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

We can keep it. No problem.

The critical part is to make sure that we have automated tests that will fail is anyone would move this code :)

docs/tutorial.rst Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
``towncrier check`` will now fail if any news fragments have invalid filenames.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this will ended up in the rendered version.

We should have 2 fragments here.
One for towncrier check

and another one for the ignore configuration option.


But maybe the towncrier check part can be left out.

If build fails, it should be expect for check to also fail.

Copy link
Contributor Author

@MetRonnie MetRonnie Jul 15, 2024

Choose a reason for hiding this comment

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

I feel like the addition of the config option and the new fail behaviour are intertwined, so I think it makes sense to have them in the same fragment. At least I think the change in behaviour of towncrier check should be clearly signposted.

I could rewrite this to be clearer, how about:

towncrier check will now fail if any news fragments have invalid filenames. You can specify filenames to ignore in the news fragments directory with the new ignore configuration option. towncrier build will not fail on invalid filenames unless ignore is set (can be an empty list).

Copy link
Member

Choose a reason for hiding this comment

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

As you think it's best

we can leave it as it is. no problem.

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. Many thanks for the changes.

@@ -101,12 +101,14 @@ def __main(
click.echo(f"{n}. {change}")
click.echo("----")

# This will fail if any fragment files have an invalid name:
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I didn't had time to check the test coverage... but if say that you have checked it, we can have this merged :)

no problem

@@ -0,0 +1,3 @@
``towncrier check`` will now fail if any news fragments have invalid filenames.
Copy link
Member

Choose a reason for hiding this comment

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

As you think it's best

we can leave it as it is. no problem.

@adiroiban adiroiban enabled auto-merge (squash) July 29, 2024 14:48
@adiroiban
Copy link
Member

Thanks for the update. I have enabled auto-merge.

I hope this will be merged soon.

I plan to do a new release soon... I hope #627 will also be merge soon and then we can have a release.

@adiroiban adiroiban merged commit 4b58be5 into twisted:trunk Jul 29, 2024
16 checks passed
@MetRonnie MetRonnie deleted the check-fragment-names branch July 29, 2024 15:18
@MetRonnie MetRonnie mentioned this pull request Jul 30, 2024
8 tasks
johannaengland added a commit to johannaengland/zino that referenced this pull request Jul 31, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to johannaengland/Argus that referenced this pull request Jul 31, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to Uninett/Argus-frontend that referenced this pull request Jul 31, 2024
johannaengland added a commit to johannaengland/nav that referenced this pull request Jul 31, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to Uninett/Argus-frontend that referenced this pull request Jul 31, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to Uninett/zino that referenced this pull request Aug 1, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to Uninett/nav that referenced this pull request Aug 1, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to Uninett/Argus-frontend that referenced this pull request Aug 1, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to Uninett/Argus that referenced this pull request Aug 1, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
johannaengland added a commit to johannaengland/zino that referenced this pull request Aug 2, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
stevepiercy added a commit to plone/volto that referenced this pull request Aug 4, 2024
Merge after the inclusion of twisted/towncrier#622 in the next release of towncrier.
jorund1 pushed a commit to jorund1/nav that referenced this pull request Aug 8, 2024
Release 24.7.0 made this necessary by scanning for invalid filenames
Reference: twisted/towncrier#622
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.

Check for invalid filenames in changes.d/ Multiple fragment types for same fragment (e.g. .bug and .bugfix)
2 participants