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

build: add CLI option 'filename' #489

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Mar 11, 2023

Description

This PR is based on:

Add CLI option --filename to allow printing the drafts anywhere other than stdout or the location defined in the configuration.

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.

@umarcor umarcor force-pushed the umarcor/cli-filename branch 2 times, most recently from 38caf30 to 5141972 Compare March 11, 2023 18:45
@codecov
Copy link

codecov bot commented Mar 11, 2023

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 99.52%. Comparing base (70da86f) to head (f2104ae).
Report is 55 commits behind head on trunk.

❗ Current head f2104ae differs from pull request most recent head 72c5327. Consider uploading reports for the commit 72c5327 to get more accurate results

Files Patch % Lines
src/towncrier/build.py 84.61% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk     #489      +/-   ##
==========================================
- Coverage   99.84%   99.52%   -0.32%     
==========================================
  Files          13       13              
  Lines         631      635       +4     
  Branches      146      149       +3     
==========================================
+ Hits          630      632       +2     
- Partials        1        3       +2     

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

@SmileyChris
Copy link
Contributor

I'm not seeing the real benefit of this. Couldn't you just pipe to a file rather than introduce another new option?

towncrier build --draft > somefile.rst

@umarcor
Copy link
Contributor Author

umarcor commented Mar 21, 2023

@SmileyChris there are at least three reasons:

  • Relying on redirection of stdout depends on the environment/language. If towncrier is called from e.g. Python (instead of bash/powershell), the syntax is different (unless the shell is used as an intermediate interpreter). Supporting it as an option provides a unified syntax, since Python itself can handle platform details transparently. Moreover, towncrier supports writing content to a file already, so we are reusing the functionality.

  • The filename is a pattern, not a hardcoded path. Therefore, variables name, version and project_date can be used. Those are 'internal' to towncrier, so extracting them would involve effort duplication.

  • In order to redirect the content to stdout, non-error messages are printed to stderr. That's not semantically correct, since it requires filtering stderr to know if unexpected content was written. See umarcor@1752e6a. I did not include that commit in this PR because that's a breaking change and I'd like to handle less conflictive changes/fixes first.

Overall, the use case I'm trying to cover is running towncrier with a filename pattern different from the one specified in the configuration and without twoncrier trying to commit any change (neither addition not removal). I considered adding an option to skip committing the file, to be used along with --keep. However, that would still require option --filename. Therefore, I thought it would be cleaner to just add that and use it along with draft. Nevertheless, I'm good with preserving draft as-is and making the two options work with "regular" builds only. I just thought that the implementation in this PR is more intuitive.

@SmileyChris
Copy link
Contributor

SmileyChris commented Mar 21, 2023

They are some solid reasons, and it does seem like a good idea.

Your considerations of just going with skipping committing files was my first thought too, but I can see benefit in using the same file output system for drafts too.

I think I like keeping the semantics of draft to only render the current changes, whether or not --filename is used. One direct example of using this could be a github action that handles creating a release and only wants the current changes as the release body (although this could also be done via piping the stdout to a file I guess :D).

(an aside though, while using stderr may be semantically incorrect, it's still pragmatically sensible. I think it's better having stdout for only data and stderr for both error messages and - for lack of additional streams - anything else that isn't data, such as progress and status information)

@umarcor
Copy link
Contributor Author

umarcor commented Mar 21, 2023

I think I like keeping the semantics of draft to only render the current changes, whether or not --filename is used.

Actually, in the use case that motivated this PR, we want to always render the body of the changes without a title. We use single_file=false and title_format=false. In that case, option using option --draft does not change the content.
We have some existing plumbing to generate the titles and some shields/hrefs at build time (https://github.com/VUnit/vunit/blob/4bbbbf2e4f8d9da1a6383d18e3f739bd5b9820ad/tools/create_release_notes.py). We only check in the bodies: https://github.com/VUnit/vunit/tree/master/docs/release_notes.

Hence, I might have overlooked something that is changing the output when title_format is not false. Is that what you mean?

One direct example of using this could be a github action that handles creating a release and only wants the current changes as the release body (although this could also be done via piping the stdout to a file I guess :D).

We considered using markdown instead of restructuredtext. That would theoretically allow us to pick the body generated by towncrier and push it as the body of the GitHub Release, apart from including it in the sphinx site (through myst). The problem we found is the issue_format. We use Sphinx extlinks to link the issues/PRs back to the repo. Those are not supported in GitHub Releases. Therefore, we would need to run towncrier twice, once with a different issue format and template. We decided to delay it until we get more familiar with the project.

(an aside though, while using stderr may be semantically incorrect, it's still pragmatically sensible. I think it's better having stdout for only data and stderr for both error messages and - for lack of additional streams - anything else that isn't data, such as progress and status information)

Since supporting --filename would suffice for our needs, I think it's not worth facing a breaking change and I believe that many users of towncrier might be relying on the current behaviour.

@umarcor umarcor force-pushed the umarcor/cli-filename branch 3 times, most recently from 7c638bf to f2104ae Compare April 12, 2023 11:11
@umarcor umarcor marked this pull request as ready for review April 12, 2023 11:13
@umarcor umarcor requested a review from a team as a code owner April 12, 2023 11:13
@umarcor
Copy link
Contributor Author

umarcor commented Apr 12, 2023

@hynek this is now ready for review!

@umarcor
Copy link
Contributor Author

umarcor commented Apr 27, 2023

With regard to the failing CI, it's complaining about the following lines not being covered: src/towncrier/build.py: 261->264, 284->296. I am confused. The default value of draft is False and the default value of filename is None. Therefore, with any default execution test both L261-L264 and L284-L296 should be triggered/used/executed.

@SmileyChris, @hynek can you please help me understand?

@SmileyChris
Copy link
Contributor

@umarcor coverage is configured to use branch testing. The -> is indicating that the case of line 261 skipping directly to 264 isn't tested.

You'll need to also ensure you're testing for the case where that if condition isn't triggered .

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 needs more tests in order to be merged.

I am not sure that --filename is needed for draft.

Would't be ok to just pipe the output to a file ?

towncrier build --draft > test-release.rst

@@ -0,0 +1 @@
Add CLI option ``--filename FILENAME``.
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 should inform that --filename was added to the build sub-command, and not to all sub-commands.

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.

3 participants