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 1 commit
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
5 changes: 5 additions & 0 deletions docs/cli.rst
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ also remove the fragments directory if now empty.

Don't delete news fragments after the build and don't ask for confirmation whether to delete or keep the fragments.

.. option:: --strict
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved

Fail if there are any news fragments that have invalid filenames.
This is automatically turned on if ``build_ignore_filenames`` has been set in the configuration.


``towncrier create``
--------------------
Expand Down
7 changes: 7 additions & 0 deletions docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,13 @@ Top level keys

``true`` by default.

``build_ignore_filenames``
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
A list of filenames in the news fragments directory to ignore when building the news file.

If this is configured, it turns on the ``--strict`` build mode which will fail if there are any news fragment files not in this list that have invalid filenames. Note that if a template is used, it should also be included here.
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved

``None`` by default.

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

Expand Down
5 changes: 5 additions & 0 deletions docs/tutorial.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ Create this folder::

The ``.gitignore`` will remain and keep Git from not tracking the directory.
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved

If needed, you can also specify a list of filenames for ``towncrier`` to ignore in the news fragments directory::
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved

[tool.towncrier]
build_ignore_filenames = ["README.rst"]


Detecting Version
-----------------
Expand Down
23 changes: 23 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,22 @@ def __call__(self, section_directory: str = "") -> str:
def find_fragments(
base_directory: str,
config: Config,
strict: bool | None,
adiroiban marked this conversation as resolved.
Show resolved Hide resolved
) -> tuple[Mapping[str, Mapping[tuple[str, str, int], str]], list[tuple[str, str]]]:
"""
Sections are a dictonary of section names to paths.

In strict mode, raise ClickException if any fragments have an invalid name.
adiroiban marked this conversation as resolved.
Show resolved Hide resolved
"""
if strict is None:
# If strict mode is not set, turn it on only if build_ignore_filenames is set
# (this maintains backward compatibility).
strict = config.build_ignore_filenames is not None

ignored_files = {".gitignore"}
adiroiban marked this conversation as resolved.
Show resolved Hide resolved
if config.build_ignore_filenames:
ignored_files.update(config.build_ignore_filenames)

get_section_path = FragmentsPath(base_directory, config)

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

for basename in files:
# Skip files that are deliberately ignored
if basename in ignored_files:
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
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 "
"'build_ignore_filenames' 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
build_ignore_filenames: list[str] | None = None


class ConfigError(ClickException):
Expand Down
16 changes: 15 additions & 1 deletion src/towncrier/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,17 @@ def _validate_answer(ctx: Context, param: Option, value: bool) -> bool:
help="Do not ask for confirmations. But keep news fragments.",
callback=_validate_answer,
)
@click.option(
adiroiban marked this conversation as resolved.
Show resolved Hide resolved
"--strict",
"strict",
default=None,
flag_value=True,
help=(
"Fail if there are any news fragments that have invalid filenames (this is "
"automatically turned on if 'build_ignore_filenames' has been set in the "
"configuration)."
),
)
def _main(
draft: bool,
directory: str | None,
Expand All @@ -115,6 +126,7 @@ def _main(
project_date: str | None,
answer_yes: bool,
answer_keep: bool,
strict: bool | None,
) -> None:
"""
Build a combined news file from news fragment.
Expand All @@ -129,6 +141,7 @@ def _main(
project_date,
answer_yes,
answer_keep,
strict,
)
except ConfigError as e:
print(e, file=sys.stderr)
Expand All @@ -144,6 +157,7 @@ def __main(
project_date: str | None,
answer_yes: bool,
answer_keep: bool,
strict: bool | None,
) -> None:
"""
The main entry point.
Expand Down Expand Up @@ -178,7 +192,7 @@ 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, strict)
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 ``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.
57 changes: 57 additions & 0 deletions src/towncrier/test/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -1530,3 +1530,60 @@ def test_orphans_in_non_showcontent_markdown(self, runner):

self.assertEqual(0, result.exit_code, result.output)
self.assertEqual(expected_output, result.output)

@with_project(
config="""
[tool.towncrier]
package = "foo"
build_ignore_filenames = ["template.jinja", "CAPYBARAS.md"]
"""
)
def test_invalid_fragment_names(self, runner):
"""
When build_ignore_filenames is set, files with those names are ignored.
"""
opts = ["--draft", "--date", "01-01-2001", "--version", "1.0.0"]
# Valid filename:
with open("foo/newsfragments/123.feature", "w") as f:
f.write("Adds levitation")
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
# Files that should be ignored:
with open("foo/newsfragments/template.jinja", "w") as f:
f.write("Jinja template")
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
with open("foo/newsfragments/CAPYBARAS.md", "w") as f:
f.write("Peanut butter")
# Automatically ignored:
with open("foo/newsfragments/.gitignore", "w") as f:
f.write("!.gitignore")

result = runner.invoke(_main, opts)
# Should succeed
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(0, result.exit_code, result.output)

# Invalid filename:
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
with open("foo/newsfragments/feature.124", "w") as f:
f.write("Extends levitation")

result = runner.invoke(_main, opts)
# Should now fail
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(1, result.exit_code, result.output)
self.assertIn("Invalid news fragment name: feature.124", result.output)

@with_project()
def test_invalid_fragment_names_strict(self, runner):
"""
When using --strict, any invalid filenames will cause an error even if
build_ignore_filenames is NOT set.
"""
opts = ["--draft", "--date", "01-01-2001", "--version", "1.0.0"]
# Invalid filename:
with open("foo/newsfragments/feature.124", "w") as f:
f.write("Extends levitation")

result = runner.invoke(_main, opts)
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
# Should succeed in normal mode
self.assertEqual(0, result.exit_code, result.output)

result = runner.invoke(_main, [*opts, "--strict"])
# Should now fail
self.assertEqual(1, result.exit_code, result.output)
self.assertIn("Invalid news fragment name: feature.124", result.output)
20 changes: 20 additions & 0 deletions src/towncrier/test/test_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,3 +470,23 @@ 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_invalid_fragment_name(self, runner):
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
create_project("pyproject.toml")
opts = ["--compare-with", "main"]

write("foo/bar.py", "# Scorpions!")
write("foo/newsfragments/123.feature", "Adds scorpions")
write("foo/newsfragments/.gitignore", "!.gitignore")
commit("add stuff")

result = runner.invoke(towncrier_check, opts)
self.assertEqual(0, result.exit_code, result.output)

# Make invalid filename:
os.rename("foo/newsfragments/123.feature", "foo/newsfragments/feature.123")

result = runner.invoke(towncrier_check, opts)
self.assertEqual(1, result.exit_code, result.output)
self.assertIn("Invalid news fragment name: feature.123", result.output)
Loading