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

Handle deletion of uncommitted news fragments #357

Merged
merged 2 commits into from
Jul 13, 2024

Conversation

abompard
Copy link
Contributor

Before this commit, all the news fragments needed to be committed into git, or the fragments removal after building the news file would crash.

In my workflow, I add missing fragments before building the news file because I'm extracting author names from the git log, and towncrier crashes at the end of the build process.

@codecov
Copy link

codecov bot commented Feb 13, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.80%. Comparing base (70da86f) to head (cb08709).
Report is 72 commits behind head on trunk.

Current head cb08709 differs from pull request most recent head 3ea6861

Please upload reports for the commit 3ea6861 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk     #357      +/-   ##
==========================================
- Coverage   99.84%   92.80%   -7.05%     
==========================================
  Files          13       11       -2     
  Lines         631      500     -131     
  Branches      146      101      -45     
==========================================
- Hits          630      464     -166     
- Misses          0       18      +18     
- Partials        1       18      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 left a few minor comments regarding the documentation/docstrings of these changes.

I would like to keep the _run helper private and outside of the git code :)

Sorry for the late review.

I hope you will still have time to drive this PR.

Let me know if you are no longer interested in working on it and I will try to take over and finalize this PR.

Cheers

src/towncrier/newsfragments/357.feature.rst 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/_git.py Outdated Show resolved Hide resolved
src/towncrier/_git.py Outdated Show resolved Hide resolved
src/towncrier/check.py Outdated Show resolved Hide resolved
@jaraco
Copy link
Contributor

jaraco commented Apr 29, 2024

It looks like this issue has gotten a little stale, and since we haven't heard back from the contributor, I'm going to close it for now. If you'd like to revive the effort, feel free to ping this PR and we can re-open, or start a new one. Thanks.

@jaraco jaraco closed this Apr 29, 2024
@abompard
Copy link
Contributor Author

Hey folks! Sorry I had totally forgotten about this PR. I'll try to find some time to finalize it, if it's still relevant.

@abompard
Copy link
Contributor Author

Could you please reopen @jaraco ?

src/towncrier/_git.py Outdated Show resolved Hide resolved
@adiroiban
Copy link
Member

Thanks for the update.

Can you please fix the conflicts and update the docstring for run to describe it's current behaviour

We no longer support python 2.7

@abompard abompard force-pushed the uncommitted branch 3 times, most recently from ec709a8 to 5ee3f19 Compare June 27, 2024 14:00
@abompard
Copy link
Contributor Author

I'm more than happy to remove Python 2.7 remnants :-D
Alright, it's updated and tests should pass now.

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

I think that we are very close to being able to merge this.

But this needs more cleanup.

The commented lines in the test file are a blocker.

Thanks again.

src/towncrier/_git.py Outdated Show resolved Hide resolved
src/towncrier/_git.py 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_build.py Outdated Show resolved Hide resolved
src/towncrier/test/test_build.py Outdated Show resolved Hide resolved
@abompard
Copy link
Contributor Author

I've made the changes, thanks a lot for your patience. FYI I do appreciate the in-depth reviews, even for minor style changes. I'm frequently on the other end of those ;-)
So thanks.

Copy link
Contributor

@SmileyChris SmileyChris left a comment

Choose a reason for hiding this comment

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

Currently, the command line docs say:

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

This should be updated. Maybe something like

By default, the processed news fragments are removed. For any fragments committed in your git repository, git rm will be used (which will also remove the fragments directory if now empty).

src/towncrier/newsfragments/357.feature.rst Outdated Show resolved Hide resolved
Before this commit, all the news fragments needed to be committed into
git, or the fragments removal after building the news file would crash.

In my workflow, I add missing fragments before building the news file
because I'm extracting author names from the git log, and towncrier
crashes at the end of the build process.

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>
@adiroiban adiroiban dismissed their stale review July 1, 2024 09:47

Chris is looking into this

@adiroiban
Copy link
Member

Thanks for the update. I will let Chris do the final review and approval for this PR.

@abompard Note that we merge with squash.
There is not much use in rewriting the history for the PR branch.
Rewriting/amending the commits on the PR make it harder to do a followup for a review.

Cheers

@abompard
Copy link
Contributor Author

abompard commented Jul 2, 2024

@abompard Note that we merge with squash. There is not much use in rewriting the history for the PR branch. Rewriting/amending the commits on the PR make it harder to do a followup for a review.

Ah, sorry about that, I didn't know.

FYI, I've had this same issue many times (trying to review PRs that rebase/rewrite) and I ended up writing a small tool to help me with the workflow: https://pypi.org/project/git-pr-branch/. The main idea is that it will download the different revisions of a PR in different branches that you can diff, even if it's always 1 commit that's being rebased.
Maybe you'll find it useful for your workflow as well.

@adiroiban
Copy link
Member

No problem.
I am looking after towncrier as my 4th job ... so I don't have much time for it.
This is why, using the embedded GitHub web tools makes it easier to review.

For most of the reviews, I don't checkout the code on my computer.
I am just doing a code review and hope the automated tests are doing their job.

@adiroiban
Copy link
Member

needs-review

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.

This looks good to me.
I will wait a bit more for Chris to review this.

If there is a big delay, we can have this merged.

There is no rush, as there is no relesed planed anytime soon...
but we might want do to a new release soon :)

@adiroiban adiroiban merged commit 8e7e240 into twisted:trunk Jul 13, 2024
16 checks passed
@adiroiban
Copy link
Member

I think that the comments from Chris were address.
I have merged this.

Thanks for your help!

@abompard abompard deleted the uncommitted branch July 23, 2024 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants