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

Provide a --version option #341

Merged
merged 1 commit into from
Apr 15, 2021
Merged

Conversation

tomschr
Copy link
Contributor

@tomschr tomschr commented Apr 14, 2021

This PR fixes #339 and adds a --version option. This makes it possible to run the command like this:

$ towncrier --version
towncrier, version 21.3.1.dev0

@tomschr
Copy link
Contributor Author

tomschr commented Apr 14, 2021

If I'm not mistaken, the failures looks like they are not directly related to my changes. I could hunt down two issues.

The first one was about some space issues:

@@ -116,6 +116,13 @@ class InvocationTests(TestCase):
             os.makedirs("news")
             out = check_output([sys.executable, "-m", "towncrier", "--help"])
             self.assertIn(b"[OPTIONS] COMMAND [ARGS]...", out)
-            self.assertIn(b"--help  Show this message and exit.", out)
+            self.assertIn(b"--help     Show this message and exit.", out)

To my understanding, if I introduce just a simple option (--version in this case) it should NOT influence this test. If it does, the test is broken. 😉

Shouldn't we write the above test in such a way that spaces donn't really matter? Isn't it possible to match against some regex like --help\s+Show this message and exit.?

The other issue was in towncrier.test.test_build.TestCli.test_command. I have no idea why it fails when I introduce the --version option.

Hope someone with more knowledge than I could look into this. Thanks! 👍

@tomschr tomschr closed this Apr 14, 2021
@tomschr tomschr reopened this Apr 14, 2021
@adiroiban
Copy link
Member

adiroiban commented Apr 14, 2021

If I'm not mistaken, the failures looks like they are not directly related to my changes. I could hunt down two issues.

I don't know. The tests were executed 7 days ago and they were green https://github.com/twisted/towncrier/runs/2288463008

I am manually triggering a re-run on the main trunk branch https://github.com/twisted/towncrier/runs/2341993285
If that run is green, then the issue is with this PR.

Shouldn't we write the above test in such a way that spaces donn't really matter? Isn't it possible to match against some regex like --help\s+Show this message and exit.?

Yes. One option is to use assertRegex

@adiroiban
Copy link
Member

adiroiban commented Apr 14, 2021

It looks like the failures are from this branch as trunk is green https://github.com/twisted/towncrier/runs/2341993285

@tomschr are the test green when you execute them on your local dev system? Are the trunk tests also green on your local dev?

Trunk is green on my local Ubuntu

(build3) ✔  ~/dev/towncrier [master|✔ ] 
$ trial towncrier

...

PASSED (successes=68)

I am checking your branch now... it fails and I am investigating the issue

@adiroiban
Copy link
Member

adiroiban commented Apr 14, 2021

The issue is TypeError('cli() takes 0 positional arguments but 1 was given') ... found on your branch via $ trial -b towncrier

I don't know how click works but you can try

diff --git a/src/towncrier/_shell.py b/src/towncrier/_shell.py
index 4023da3..bc91861 100644
--- a/src/towncrier/_shell.py
+++ b/src/towncrier/_shell.py
@@ -17,7 +17,7 @@ from ._version import __version__
 @click.group(cls=DefaultGroup, default="build", default_if_no_args=True)
 @click.version_option(__version__.public())
 @click.pass_context
-def cli():
+def cli(*args, **kwargs):
     pass

src/towncrier/_shell.py Outdated Show resolved Hide resolved
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 think that it looks good.... but it needs some cleanup.

Do we need @click.pass_context?
I have removed it and test_version was still green.

Would be nice to use assertRegex instead of assertIn but I am also ok with your quick fix for assertIn :)

Thanks!

@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #341 (8924ee6) into master (b8a3490) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 8924ee6 differs from pull request most recent head d5dae8d. Consider uploading reports for the commit d5dae8d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   96.78%   96.79%   +0.01%     
==========================================
  Files          20       20              
  Lines        1182     1187       +5     
  Branches      106      106              
==========================================
+ Hits         1144     1149       +5     
  Misses         20       20              
  Partials       18       18              
Impacted Files Coverage Δ
src/towncrier/_shell.py 100.00% <100.00%> (ø)
src/towncrier/test/test_project.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8a3490...d5dae8d. Read the comment docs.

@tomschr
Copy link
Contributor Author

tomschr commented Apr 14, 2021

Hi @adiroiban,
thanks for the feedback, much appreciated! 👍 I've added your suggestions and on my machine it works and all is green. I've used the assertRegex part.

Should I squash the commits or are you doing it? 🙂

Thanks again!

@adiroiban
Copy link
Member

adiroiban commented Apr 14, 2021

You can squash the commit and don't forget to add a news fragment inside src/towncrier/newsfragments/ for towncrier itself :)t

This is why CI / Check Newsfragment is red.

It can be src/towncrier/newsfragments/339.feature with content like

`towncrier --version` was added to the command line interface for showing the product version.

@tomschr tomschr force-pushed the feature/339-version branch from 8924ee6 to b5f7d3e Compare April 14, 2021 12:09
@tomschr
Copy link
Contributor Author

tomschr commented Apr 14, 2021

Seems this time I was successful. 😄 Adi, anything else I should add, correct etc.?

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.

Look good. Only a few minor comments :) Thanks!

src/towncrier/newsfragments/339.feature.rst Outdated Show resolved Hide resolved
src/towncrier/test/test_project.py Outdated Show resolved Hide resolved
src/towncrier/test/test_project.py Outdated Show resolved Hide resolved
* Create and adapt tests for `--version`
* Add newsfragment

Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@tomschr tomschr force-pushed the feature/339-version branch from 170ff02 to d5dae8d Compare April 14, 2021 14:53
@adiroiban adiroiban merged commit 61681d6 into twisted:master Apr 15, 2021
@altendky
Copy link
Member

Thanks to you both. :]

@tomschr tomschr deleted the feature/339-version branch April 15, 2021 21:47
@seberg
Copy link

seberg commented Apr 16, 2021

I think this PR broke our CI at NumPy, because --version is a valid option to overwrite the "version" identifier in the release notes. (Still using the dev version, although I think there was probably a new enough release by now)

This is mainly to let you know that this breaks (maybe add a release note). I can fix it by using towncrier build to make it explicit that we are building.

@adiroiban
Copy link
Member

Thanks @seberg for the report. Much appreciated and it's an important side effect.

I guess that in order to not break backward compatibility we can add towncrier version

But maybe we should should just go with towncrier subcommands and in this case we need to update the release note and inform about this backward incompatible change.

@altendky
Copy link
Member

The default subcommands are nice for backwards compatibility, but maybe when we add them the implicit usage should be deprecated and later removed. I'm not used to using default subcommands so definitely thanks for filling that gap in my thinking and reporting this @seberg.

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.

Consider a --version option
4 participants