-
Notifications
You must be signed in to change notification settings - Fork 106
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
Allow for defining arbitrary project version and name in configuratio… #166
Allow for defining arbitrary project version and name in configuratio… #166
Conversation
Codecov Report
@@ Coverage Diff @@
## master #166 +/- ##
==========================================
+ Coverage 89.49% 89.59% +0.09%
==========================================
Files 11 11
Lines 438 442 +4
Branches 87 89 +2
==========================================
+ Hits 392 396 +4
Misses 25 25
Partials 21 21
Continue to review full report at Codecov.
|
We've successfully cherry-picked the changes in this PR to Oasis Labs' towncrier fork and used it with the Oasis Core repo, which is a Go & Rust project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs a catch up. Anyways, thanks for the work. Sorry I went and duplicated it, though that made the review easy since they were so close. :]
I can't merge anything right now but I'll try to get that fixed so we can get this integrated once we work through the minor tidbits. My apologies it has taken so long to even get any feedback.
src/towncrier/_settings.py
Outdated
"project_version": config.get("project_version"), | ||
"project_name": config.get("project_name"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the names in the configuration file need the project_
prefix? Or would name
and version
suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a matter of preference, and short version is as good as longer. One does not use library_version or library_name variables after all...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although comming back, I used the project_ prefixes since I was extending the use of already existing variables in code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw the code used longer names and that makes sense to me. I couldn't make up my mind so I thought about setuptools and checked Poetry and both of them use just name
and version
. I don't think it ends up being ambiguous. I don't feel strongly enough to push hard on this, but if you are comfortable with changing to the shorter, I think that's a touch more straightforward. Take your pick and we'll move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, changed to shorter
…n - close twisted#165 This allows for the towncrier to be seamlessly used in non-python projects
2a1e0b0
to
9957da2
Compare
Co-authored-by: Kyle Altendorf <sda@fstab.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started looking through the readme and found a few other references to 'version' that seemed like they could use some updating to account for this new feature.
So I just realized that this also introduces the possibility of specifying the version and the name via both the command line options and the configuration file. I think that other options, at least on the build
command, can't do that. I vaguely feel like perhaps the CLI options should go away, though there would be a deprecation period for that. Let's just ignore that for now.
It can be handled in a separate PR if needed. But, it would probably be good to add tests that confirm the priority of the command line over the configuration file.
With all this time since you submitted this, I'm glad you are available to get it worked through and finished up quickly. :]
I'm sorry I keep finding tidbits. I'm still on the newer side of review and appreciate your patience.
src/towncrier/_settings.py
Outdated
"project_version": config.get("project_version"), | ||
"project_name": config.get("project_name"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw the code used longer names and that makes sense to me. I couldn't make up my mind so I thought about setuptools and checked Poetry and both of them use just name
and version
. I don't think it ends up being ambiguous. I don't feel strongly enough to push hard on this, but if you are comfortable with changing to the shorter, I think that's a touch more straightforward. Take your pick and we'll move on.
Co-authored-by: Kyle Altendorf <sda@fstab.net>
Also, I'd gladly squash commits and rework commit message if that'd be something you want. |
My personal preference is to never change public history, so no need to go squashing etc for my sake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a few references to 'version' in the readme that will mislead people into thinking there isn't a new option for specifying it. If you would rather, I could go through that. Just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see ClearcodeHQ#2 since GitHub didn't let me add a suggestion for a line not already in the diff context.
I think maybe that's it.
Co-authored-by: Kyle Altendorf <sda@fstab.net>
Co-authored-by: Kyle Altendorf <sda@fstab.net>
215a80a
to
833c8cf
Compare
@fizyk, sorry for the delay here. Looks good to me. I've got the CI fixed up properly now so I'm going to go ahead and update the branch here from master. Hopefully that'll just work and I'll merge this but I wanted to give you a heads up so you don't go trying to push new work without pulling and get a surprise. |
@altendky thank you! |
…n - close #165