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
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ Top level keys

``true`` by default.

``ignore``
A case-insensitive list of filenames in the news fragments directory to ignore.

``towncrier check`` will fail if there are any news fragment files that have invalid filenames, except for those in the list. ``towncrier build`` will likewise fail, but only if this list has been configured (set to an empty list if there are no files to ignore).

Some filenames such as ``.gitignore`` and ``README`` are automatically ignored. However, if a custom template is stored in the news fragment directory, you should add it to this list.

``None`` by default.

Extra top level keys for Python projects
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
2 changes: 0 additions & 2 deletions docs/tutorial.rst
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ Create this folder::
# This makes sure that Git will never delete the empty folder
$ echo '!.gitignore' > src/myproject/newsfragments/.gitignore

The ``.gitignore`` will remain and keep Git from not tracking the directory.


Detecting Version
-----------------
Expand Down
17 changes: 17 additions & 0 deletions src/towncrier/_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from pathlib import Path
from typing import Any, DefaultDict, Iterable, Iterator, Mapping, NamedTuple, Sequence

from click import ClickException
from jinja2 import Template

from towncrier._settings.load import Config
Expand Down Expand Up @@ -106,10 +107,17 @@ def __call__(self, section_directory: str = "") -> str:
def find_fragments(
base_directory: str,
config: Config,
strict: bool,
) -> tuple[Mapping[str, Mapping[tuple[str, str, int], str]], list[tuple[str, str]]]:
"""
Sections are a dictonary of section names to paths.

If strict, raise ClickException if any fragments have an invalid name.
"""
ignored_files = {".gitignore", ".keep", "readme", "readme.md", "readme.rst"}
if config.ignore:
ignored_files.update(filename.lower() for filename in config.ignore)

get_section_path = FragmentsPath(base_directory, config)

content = {}
Expand All @@ -129,10 +137,19 @@ def find_fragments(
file_content = {}

for basename in files:
if basename.lower() in ignored_files:
Copy link
Member

Choose a reason for hiding this comment

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

It looks like with this change, the coverage if failing for line 30 :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, I think my latest commit should sort that

continue

issue, category, counter = parse_newfragment_basename(
basename, config.types
)
if category is None:
if strict and issue is None:
raise ClickException(
f"Invalid news fragment name: {basename}\n"
"If this filename is deliberate, add it to "
"'ignore' in your configuration."
)
continue
assert issue is not None
assert counter is not None
Expand Down
1 change: 1 addition & 0 deletions src/towncrier/_settings/load.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class Config:
orphan_prefix: str = "+"
create_eof_newline: bool = True
create_add_extension: bool = True
ignore: list[str] | None = None


class ConfigError(ClickException):
Expand Down
8 changes: 7 additions & 1 deletion src/towncrier/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,13 @@ def __main(

click.echo("Finding news fragments...", err=to_err)

fragment_contents, fragment_files = find_fragments(base_directory, config)
fragment_contents, fragment_files = find_fragments(
base_directory,
config,
# Fail if any fragment filenames are invalid only if ignore list is set
# (this maintains backward compatibility):
strict=(config.ignore is not None),
)
fragment_filenames = [filename for (filename, _category) in fragment_files]

click.echo("Rendering news fragments...", err=to_err)
Expand Down
4 changes: 3 additions & 1 deletion src/towncrier/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

I don't think that this comment is needed.

The docstring for find_fragments should be enough of a documentation.

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 added this comment to make sure anyone thinks twice before possibly moving this back down below to just before the result is used (because the next part could lead this this check being skipped)

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 :)

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 think this will fail because of the control case fragment, but I haven't tried moving the line back down and re-running

def test_invalid_fragment_name(self, runner):
"""
Fails if a news fragment has an invalid name, even if `ignore` is not set in
the config.
"""
create_project("pyproject.toml")
write(
"foo/newsfragments/123.feature",
"This fragment has valid name (control case)",
)

Copy link
Member

Choose a reason for hiding this comment

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

We need to check this, as the test are the critical part of a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (test updated to ensure)

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

all_fragment_files = find_fragments(base_directory, config, strict=True)[1]

news_file = os.path.normpath(os.path.join(base_directory, config.filename))
if news_file in files:
click.echo("Checks SKIPPED: news file changes detected.")
sys.exit(0)

all_fragment_files = find_fragments(base_directory, config)[1]
fragments = set() # will only include fragments of types that are checked
unchecked_fragments = set() # will include fragments of types that are not checked
for fragment_filename, category in all_fragment_files:
Expand Down
3 changes: 3 additions & 0 deletions src/towncrier/newsfragments/622.feature.rst
Original file line number Diff line number Diff line change
@@ -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.


Added a new configuration option called ``ignore`` 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.
63 changes: 63 additions & 0 deletions src/towncrier/test/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -1583,3 +1583,66 @@ def test_uncommitted_files(self, runner, commit):
"""
),
)

@with_project(
config="""
[tool.towncrier]
package = "foo"
ignore = ["template.jinja", "CAPYBARAS.md"]
"""
)
def test_ignored_files(self, runner):
"""
When `ignore` is set in config, files with those names are ignored.
"""
with open("foo/newsfragments/123.feature", "w") as f:
f.write("This has valid filename (control case)")
with open("foo/newsfragments/template.jinja", "w") as f:
f.write("This template has been manually ignored")
with open("foo/newsfragments/CAPYBARAS.md", "w") as f:
f.write("This markdown file has been manually ignored")
with open("foo/newsfragments/.gitignore", "w") as f:
f.write("gitignore is automatically ignored")

result = runner.invoke(
_main, ["--draft", "--date", "01-01-2001", "--version", "1.0.0"]
)
self.assertEqual(0, result.exit_code, result.output)

@with_project(
config="""
[tool.towncrier]
package = "foo"
ignore = []
"""
)
def test_invalid_fragment_name(self, runner):
"""
When `ignore` is set in config, invalid filenames cause failure.
"""
with open("foo/newsfragments/123.feature", "w") as f:
f.write("This has valid filename (control case)")
with open("foo/newsfragments/feature.124", "w") as f:
f.write("This has the issue and category the wrong way round")

result = runner.invoke(
_main, ["--draft", "--date", "01-01-2001", "--version", "1.0.0"]
)
self.assertEqual(1, result.exit_code, result.output)
self.assertIn("Invalid news fragment name: feature.124", result.output)

@with_project()
def test_no_ignore_configured(self, runner):
"""
When `ignore` is not set in config, invalid filenames are skipped.

This maintains backward compatibility with before we added `ignore`
to the configuration spec.
"""
with open("foo/newsfragments/feature.124", "w") as f:
f.write("This has the issue and category the wrong way round")

result = runner.invoke(
_main, ["--draft", "--date", "01-01-2001", "--version", "1.0.0"]
)
self.assertEqual(0, result.exit_code, result.output)
44 changes: 44 additions & 0 deletions src/towncrier/test/test_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,3 +470,47 @@ def test_in_different_dir_with_nondefault_newsfragments_directory(self, runner):
self.assertTrue(
result.output.endswith("No new newsfragments found on this branch.\n")
)

@with_isolated_runner
def test_ignored_files(self, runner):
"""
When `ignore` is set in config, files with those names are ignored.
"""
create_project("pyproject.toml", extra_config='ignore = ["template.jinja"]')

write(
"foo/newsfragments/124.feature",
"This fragment has valid name (control case)",
)
write("foo/newsfragments/template.jinja", "This is manually ignored")
write("foo/newsfragments/.gitignore", "gitignore is automatically ignored")
commit("add stuff")

result = runner.invoke(towncrier_check, ["--compare-with", "main"])
self.assertEqual(0, result.exit_code, result.output)

@with_isolated_runner
def test_invalid_fragment_name(self, runner):
"""
Fails if a news fragment has an invalid name, even if `ignore` is not set in
the config.
"""
create_project("pyproject.toml")

write(
"foo/newsfragments/124.feature",
"This fragment has valid name (control case)",
)
write(
"foo/newsfragments/feature.125",
"This has issue and category wrong way round",
)
write(
"NEWS.rst",
"Modification of news file should not skip check of invalid names",
)
commit("add stuff")

result = runner.invoke(towncrier_check, ["--compare-with", "main"])
self.assertEqual(1, result.exit_code, result.output)
self.assertIn("Invalid news fragment name: feature.125", result.output)
Loading