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

Fix orphan fragments with numbers #564

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

mdellweg
Copy link
Contributor

@mdellweg mdellweg commented Nov 7, 2023

Description

Fixes #562

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.

@adiroiban
Copy link
Member

adiroiban commented Nov 7, 2023

We can discuss the changes here.

The test changes looks great. It should be enought to update the existing tests.

No need for a new tests... a new test would be nice... but the current one is also ok.

I see the updated tests failing. THat's excellent.

Not sure why 3.9 wasn't failing and also mypy 3.11 (it was the build job, not the test one)

@@ -17,6 +17,8 @@
.vs/
.vscode
Justfile
*egg-info/
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can have this. I have this rule in my global gitignore file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call.
Also this is a python project, so i'd expect that directory to exist eventually.

.gitignore Outdated Show resolved Hide resolved
@mdellweg mdellweg changed the title WIP: Fix orphan fragments with numbers Fix orphan fragments with numbers Nov 7, 2023
@@ -70,9 +78,13 @@ def _test_command(self, command):
--------

- Baz levitation (baz)
- Baz fix levitation (#2)
- Baz fix levitation (fix-1.2)
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'm not so sure about this one. But i'd argue, that is what was actually meant by:
https://github.com/twisted/towncrier/pull/564/files#diff-36db088409efe07783d49792a18cec6ae3778ace338bf1cd08daa8a3fce29e93R45-R47

# NOTE: This allows news fragment names like fix-1.2.3.feature or
# something-cool.feature.ext for projects that don't use ticket
# numbers in news fragment names.

@mdellweg mdellweg marked this pull request as ready for review November 7, 2023 10:11
@mdellweg mdellweg requested a review from a team as a code owner November 7, 2023 10:11
@@ -70,9 +78,13 @@ def _test_command(self, command):
--------

- Baz levitation (baz)
- Baz fix levitation (#2)
- Baz fix levitation (fix-1.2)
Copy link
Member

Choose a reason for hiding this comment

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

I think that this need a better comment.

I don't really understant what is the use case here.

This is the bad part of bundling all kind of scenarios in a single test case.

            # Towncrier supports files that have a dot in the name of the
            # newsfragment
            with open("foo/newsfragments/fix-1.2.feature", "w") as f:
                f.write("Baz fix levitation")

since it has no orphan marker, I would consider it an error, and the file should be rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per my comment above, I think this snippet was parsed wrongly. But I cannot find anything in the docs about it the only reference is the comment (_builder.py):

# NOTE: This allows news fragment names like fix-1.2.3.feature or
# something-cool.feature.ext for projects that don't use ticket
# numbers in news fragment names.

And I do not think that in that case "3" was meant to be an issue number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I read the comment you cited as the "name of the newsfragment" being fix-1.2.

@@ -405,6 +417,7 @@ def test_draft_no_date(self):
call(["git", "init"])
call(["git", "config", "user.name", "user"])
call(["git", "config", "user.email", "[email protected]"])
call(["git", "config", "commit.gpgSign", "false"])
Copy link
Member

@adiroiban adiroiban Nov 7, 2023

Choose a reason for hiding this comment

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

We might need to refactor this code into an helper function that does the git init for us.

I expect that this is needed in other tests.

We can leave this as it is for now.

There are numerous bad practices in the towncrier tests.
It would be nice to have them fix, but it should not be a burder for new contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this?

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 the PR.
I did a review now as I had some time.

This needs changes due to test_dots_in_ticket_name

This change introduces a regression.

If we decide that test_dots_in_ticket_name was a bug, we need to add a .removal news fragment for this PR to let others know that this use case is no longer supported and that it was removed.

But, it looks like test_dots_in_ticket_name is indeed a feature... it's just that it was not documented.

So we need to decide if we want to keep the undocumented feature, add documentation for it, or plan to remove this feature.

I hope that we can keep this feature.

@@ -8,63 +8,73 @@

class TestParseNewsfragmentBasename(TestCase):
def test_simple(self):
"""Plain <number>.<category> should be valid with counter=0."""
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 leave these comments.

Just a note, if you have time to make them better

I recommend this read ... is not log https://jml.io/test-docstrings/

Maybe

Suggested change
"""Plain <number>.<category> should be valid with counter=0."""
"""
<number>.<category> generates a counter value of 0.
"""

@@ -69,3 +70,43 @@ def setup_simple_project(

if mkdir_newsfragments:
Path("foo/newsfragments").mkdir()


def with_isolated_simple_project(
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the refactoring.

I know that we already have setup_simple_project

The test decorators are all about isolation.

And not sure if "simple" has value.

Do we also have "with_isolated_complex_project" ?

Suggested change
def with_isolated_simple_project(
def with_project(

Can we have a docstring for this function?

I think that in the docstring we can have more details about what is considered "simple".

My main goal for the docstring is to describe the intended use cases for each argument.

With that, you can decide if you need to extend this function, or create a new one (that for example is more complex)

result = runner.invoke(command, ["--draft", "--date", "01-01-2001"])
@with_isolated_simple_project()
def _test_command(self, command, runner):
# Off the shelf newsfragment
Copy link
Member

Choose a reason for hiding this comment

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

What does this comment mean ?

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 consider this the archetype of a newsfragment. (One line down; this is not a docstring...) How would you call it?


def with_isolated_simple_project(
*,
with_git=False,
Copy link
Member

Choose a reason for hiding this comment

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

I am not a big fan of flag arguments.

I prefer something like @with_project and @with_git_project . but we can have this like this, as long as the intended usage of with_git is documented.

def with_isolated_simple_project(
*,
with_git=False,
config: str | None = None,
Copy link
Member

@adiroiban adiroiban Nov 7, 2023

Choose a reason for hiding this comment

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

maybe config can be dedented by default, so that we don't have to always call dedent.

By forcing the usage of dedent, it should make the tests easier to read.
We no longer have to do list concatenation

*,
with_git=False,
config: str | None = None,
extra_config: str = "",
Copy link
Member

Choose a reason for hiding this comment

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

Just a comment. Not sure if it make sense to keep this.

I find it easier to read a test, if it has the full configuration.

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.

Changes look very good. Many thanks.

I think that the only major comment is the missing release note for the fix.1.2.3.bugfix change.

We should add a note about this in the relese note, so that people who were accidentally relying on this bug, should know about it and switch to orphan marker.

The others are just minor comments. Nothing to block the merge.

Thanks again!

@mdellweg
Copy link
Contributor Author

mdellweg commented Nov 7, 2023

I'm going to separate the test rewrite into a different PR for easier review.

@adiroiban
Copy link
Member

I'm going to separate the test rewrite into a different PR for easier review.

That would be awesome. Much appreciated.

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.

I am not sure we have a removal here.
I think there are just 2 bugfixes.

@mdellweg
Copy link
Contributor Author

mdellweg commented Nov 7, 2023

I believe i updated the fragments as you wanted (tried to fix any typo found...).

@adiroiban adiroiban merged commit 3b27ce2 into twisted:trunk Nov 7, 2023
13 checks passed
@adiroiban
Copy link
Member

Many thanks. Great work. I have merged it and I will try to get a new release

@mdellweg mdellweg deleted the 562_adopted_orphan branch November 7, 2023 16:06
@mdellweg
Copy link
Contributor Author

mdellweg commented Nov 7, 2023

Thank you for the quick reviews!

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.

orphan fragments that contain a number in the filename are linked to issues.
2 participants