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.

Sign up for GitHub

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

Jump to bottom

Make consistent the expectation around non-existent fragments directory #557

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Make consistent the expectation around non-existent fragments directory #557

merged 3 commits into from
Oct 24, 2023

Conversation

jaraco
Copy link
Contributor

@jaraco jaraco commented Oct 24, 2023 •
edited
Loading

Description

In the first commit, I update the documentation to reflect the current behavior, highlighting that a missing fragments directory is normal for a "create" operation and caused by the "build" operation.

In the second commit, I expand the documentation to include the case where "build" is invoked and the fragments directory is missing to be consistent with an empty directory or no directory at all. I also draft a likely way to achieve this behavior. Once tests pass, this approach closes #538.

Note that this change aligns the behavior with the currently prescribed expectation.

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.

@jaraco jaraco requested a review from a team as a code owner October 24, 2023 01:08
@jaraco jaraco force-pushed the feature/publish-nonexistent-fragments branch 3 times, most recently from 4bef7d6 to a5bf1ac Compare October 24, 2023 01:20
adiroiban
adiroiban approved these changes Oct 24, 2023
View reviewed changes
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 very good. Many thanks.

Only minor comments.

src/towncrier/test/test_build.py
self.assertEqual(1, result.exit_code, result.output)
self.assertIn("Failed to list the news fragment files.\n", result.output)
self.assertEqual(0, result.exit_code)
self.assertIn("No significant changes.\n", result.output)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. It looks like this was a bug.

The test docstring states that it is expected to handle it as no changes, but in fact it was failing :)

docs/cli.rst
Comment on lines +31 to +32
By default, the processed news fragments are removed using ``git``, which will
also remove the fragments directory if now empty.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By default, the processed news fragments are removed using ``git``, which will
also remove the fragments directory if now empty.
By default, the processed news fragments are removed using ``git rm``.

I don't think that towncrier (or git) explicitly removes the empty dir.

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 tested this expectation in one of my projects. For example, you can checkout /jaraco/skeleton, which currently has one fragment, and run towncrier build 0.0.0, and it will remove the newsfragmenta dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's where I did just that yesterday:

 @ work-on skeleton
 skeleton main @ towncrier build --version 0.0.0
Loading template...
Finding news fragments...
Rendering news fragments...
Writing to newsfile...
Staging newsfile...
I want to remove the following files:
/Users/jaraco/code/jaraco/skeleton/newsfragments/+drop-py37.feature.rst
Is it okay if I remove those files? [Y/n]: y
Removing news fragments...
Done!
 skeleton main @ cat NEWS.rst
0.0.0
=====

Features
--------

- Require Python 3.8 or later.
 skeleton main @ towncrier build --version 0.0.1
Loading template...
Finding news fragments...
Failed to list the news fragment files.
FileNotFoundError: [Errno 2] No such file or directory: '/Users/jaraco/code/jaraco/skeleton/newsfragments'

 skeleton main @ mkdir newsfragments
 skeleton main @ towncrier build --version 0.0.1
Loading template...
Finding news fragments...
Rendering news fragments...
Writing to newsfile...
Staging newsfile...
No news fragments to remove. Skipping!
Done!

src/towncrier/newsfragments/538.feature.rst Outdated Show resolved Hide resolved
@jaraco jaraco force-pushed the feature/publish-nonexistent-fragments branch from a5bf1ac to ed02443 Compare October 24, 2023 12:01
@jaraco jaraco force-pushed the feature/publish-nonexistent-fragments branch from ed02443 to c36652f Compare October 24, 2023 12:02
@adiroiban adiroiban merged commit e4b892f into twisted:trunk Oct 24, 2023
13 checks passed
@adiroiban
Copy link
Member

Thanks. Sorry for the extra trouble. I have merged it.

@jaraco jaraco deleted the feature/publish-nonexistent-fragments branch October 24, 2023 15:59
@iliakur
Copy link
Contributor

iliakur commented Nov 2, 2023

@adiroiban you're gonna laugh, but I think we ran into this situation as well. I think we're gonna change our process for other reasons in a way that this isn't a problem. However, just in case, would you mind cutting a release with this in the next few days? Thanks a bunch!!

@adiroiban
Copy link
Member

@iliakur here is the release PR #566

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiroiban adiroiban adiroiban approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FileNotFoundError, Failed to list the news fragment files
3 participants
@jaraco @adiroiban @iliakur