-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
python: Switch to pyproject.toml #290
Conversation
I had to remove the `bin/gherkin` executable since that doesn't work with pyproject.toml (scripts can only be python entrypoints). But I replaced with the correct annotation in pyproject.toml, so if you install the package, the script will be automatically installed in the venv too
Just a comment:
|
RELATED TO:
|
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.
This line is probably wrong and you need a main()
function in gherkin.__main__.py
(AFAIK; or any other function that you can refer to).
See comment in the comments section.
We don't need to distribute these scripts as entry points. It's unlikely that any of our users need these, as they are just used internally for acceptance test. Instead, let's move them to the package `gherkin.scripts`, and invoke them using `python -m gherkin.scripts.xxx`
So, given these scripts are only used internally by acceptance tests, I removed them from the entrypoints, so that we wont't try to install the I adjusted our makefile accordingly to instead call |
@jenisys can you review again? |
REVIEWED AGAIN: see the comments. |
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.
See comments:
- Package name was corrected to original package name.
- Specify package scripts in
pyproject.toml
instead of leaving the user alone. -
gherkin/__main__.py
should be kept as natural choice for the main program.
NOTE: You may refer to themain()
function ingherkin.scripts.gherkin
if you prefer that and keep the logic there. - Find package mechanism that should fix that subpackages are left out (currently the case)
- Replace
raise SystemExit(main())
withsys.exit(main())
@@ -22,6 +22,7 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt | |||
|
|||
### Removed | |||
- [Python] Drop compatibility for python 2. Supported python versions are 3.8, 3.9, 3.10, 3.12 | |||
- [Python] Removed installation of `gherkin` script. It was used for internal acceptance tests only. |
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.
BAD IDEA: Removing the scripts from the package description
- The user of this package wants to use the scripts and not some module statements that he/she does not know
- Using
gherkin/__main__.py
should be kept because that is the natural choice for the main program - Usability for the user sucks if you do that
- It might be OK for the repo development in Makefile but even there, it is rather suboptimal (IMHO).
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.
actually, I'm even more inclined to not include the scripts/
folder in the distribution at all.
The reason is that these scripts are only used internally, as there is absolutely no API or documentation about these. Hell, I can't even figure out what the gherkin
script does at all, since there is no documentation.
I really doubt that any downstream user is using these. If anything, we are just polluting their PATH with this irrelevant gherkin
executable.
In the remote hypothesis that a downstream user was relying on this, we can always release a hotfix with the script included in the distribution.
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.
If the executable is needed for the purposes of the user to understand how to run main, wouldn't this be better handled with documentation?
I may have missed something, but I didn't use this executable or use it to understand how to use gherkin when I implemented it in pytest-bdd. I'm not entirely sure how useful it for including downstream from personal experience, unless I've missed a point.
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.
If these scripts/executables are only for internal use, then why put them in the Python package and not put the in bin/
directory where they were before (at least one of them).
In addition, the gherkin
script is provided in the current package version and before. Therefore, the script was part of the “API” of this package. By removing them you basically break this API in the next version. And you should be aware of that.
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.
If these scripts/executables are only for internal use, then why put them in the Python package and not put the in bin/ directory where they were before (at least one of them
I can put them out of the gherkin
package, so that they will automatically be out of the distributed package.
In addition, the gherkin script is provided in the current package version and before. Therefore, the script was part of the “API” of this package. By removing them you basically break this API in the next version. And you should be aware of that.
yes I know, but every release is a major release it seems, so semver semantics allow this.
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 can put them out of the gherkin package, so that they will automatically be out of the distributed package.
Done in b94911b
|
||
|
||
if __name__ == "__main__": | ||
raise SystemExit(main()) |
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.
BAD IDEA: raise SystemExit(main())
— Raising an exception and calling the main()
function as side-effect
- Use
sys.exit(main())
instead
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.
sys.exit(...)
and raise SystemExit(...)
are completely equivalent, so this is only about style.
I prefer the latter just because it avoids an import. Wdyt?
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.
calling the main() function as side-effect
This is not really a side-effect. You may argue that also in sys.exit(main())
it is a side effect.
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.
Agreed this is mainly a styling thing from what I can see, unless I am mistaken?
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.
Actually, it is not. It is the difference between using the official API for something and programming by side-effects.
- Show me one example in https://docs.python.org where your idiom is used in the context of a
__main__
section - Show me one example why packaging generates scripts the way I describe instead of how you use it
SEE ALSO:
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.
How is this programming by side effect? Where is the side effect here?
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.
anyway, I removed raise SystemExit
in 5405b62 to make everyone happy
python/gherkin/scripts/gherkin.py
Outdated
|
||
|
||
if __name__ == "__main__": | ||
raise SystemExit(main()) |
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.
BAD IDEA: raise SystemExit(main())
— Explanation, see above.
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.
removed in 5405b62
python/pyproject.toml
Outdated
gherkin = ["gherkin-languages.json"] | ||
|
||
[tool.setuptools.packages.find] | ||
include = ["gherkin", "gherkin.pickles", "gherkin.stream"] |
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.
BETTER: Use the find
mechanism that is described in setuptools docs: Find simple packages
- BLOCKER: In your case the
gherkin/scripts/
directory would have been missing.
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.
done in d992ac4
No need to distribute them, as they are for internal tests
…ing `pyproject.toml`
I looked over the current state again. MAJOR:
MINOR:
|
it does:
|
good one, deleted in 987e836 |
It always did. That's the expected behaviour. |
Removed |
@jenisys please review again |
RELATED TO:
|
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.
DONE: See comments.
Why is it weird?
An source for this? To me, the sdist should just contain enough info to build the package, not to test it. If we want to make the sdist testable, we should include all the testdata from upper level too. Otherwise it would just be half done. |
* Provide "pyproject.toml" that supersedes "setup.py" * Moved scripts from "bin/" to "scripts" directory * CHANGELOG: Add mention of pull #290 for Python change. CLEANUP: * Delete leftover `MANIFEST`, which is not necessary anymore * Remove "setup.py"
MERGED INTO: main (manually and squashed commits, shortened commit-message) |
Fixes #289
Notes:
I had to remove the
bin/gherkin
executable since that doesn't work with pyproject.toml (scripts can only be python entrypoints). But I replaced with the correct annotation in pyproject.toml, so if you install the package, the script will be automatically installed in the venv tooI uploaded the package built using
python -m build
to https://test.pypi.org/project/youtux.gherkin-official/, in case somebody wants to check how it will look on PyPI or to try it out.🤔 What's changed?
⚡️ What's your motivation?
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.