-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Cucumber Expressions for Python #65
Conversation
I may have missed some bits, but wanted to get the first "draft" out for feedback. I can't work out the only test that fails: test_prefers_expression_with_longest_non_empty_match |
I just look over your pull-request (not the contents, just the filename). You violated the PEP-8 naming conventions for Python by using CamelCase instead of snake_case for Python packages/modules.
SEE ALSO: OTHERWISE:
|
Great feedback, @jenisys - thanks! I am not an expert in Python so thought I would pop what I have done so far rather than getting too far down the line and not realising my mistakes. I will have a look at what you have suggested - having never released any artifacts for python, all that area and nomenclature are new territory. Many thanks again! |
No problem, learning by doing. |
Outstanding effort @jsa34 , thanks so much for contributing. I rounded up some pythonistas to help with this effort just before the holidays and I think everyone was too busy to take action. I would be very happy to work with you if you are interested in any kind of collaboration.
|
Hello! That's great and would be very much appreciated. I tried to keep as close to the other lang implementations as possible with a sprinkling of pythonisms where possible, but where the line is drawn I'm not sure as the closer it is to Ruby/JS, etc I guess maintainability for cross language feature is better, but again I deliberately thought I would share at this point so I can start discussions and gather feedback. I would very much appreciate any support/collaboration as it will most definitely help me learn more about Python and hopefully help me become more "pythonic"! Let me know what you suggest :) |
Added missing tests
Some tidy up of code to keep the code cleaner
Some tidy up of code to keep the code cleaner
Spelling fix
@jsa34 , I would say let's go ahead and I think that @mattwynne is correct is wanting to make the language-specific implementations representative of the native customs. |
@blaisep Resolved merge conflicts. Happy to make this reflect more the language native customs, but if you are able to provide any guidance/examples of this that would be gratefully appreciated! Happy to chat about it or to go through code comments |
Type parameter is called "param_type" rather than type so as to not have the possibility of shadowing native 'type'.
…c implementation where possible
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.
Excellent work @jsa34 - thank you!
Could you please add build scripts and/or instructions so other contributors can run all tests with a single command after cloning the repo?
- Maybe add a
requirements.txt
file that lists dependencies - How to run all the tests
Could you also please add a GitHub Action that does all of the above? You can look at this one for inspiration: https://github.com/cucumber/tag-expressions/blob/main/.github/workflows/test-python.yml
@jenisys I'm not sure it's a good idea to rely on make for this (Windows users don't always have it). What's the most idiomatic and cross platform tool in Python to write build scripts?
…k that was committed since I started...
Hopefully this works!
@aslakhellesoy (question from review above):
Correct, using "make" normally does not work on Windows for many users. Otherwise, if a "pyproject.toml" is provide (newer mechanism instead of "setup.py") to be able to install the package, BUT: Just for testing it, a simpler solution would be a short description in the README. $ cd cucumber-expressions/python/
# -- BETTER: Use "requirements.txt" (or: "py.requirements.txt")
# HINT: Newer pip versions install under $HOME
# MAYBE NEEDED: PATH="$HOME/Library/Python/3.9/bin:$PATH" (for installed python scripts on my Mac)
$ pip install pytest coverage PyYAML
# -- Run the tests:
$ pytest
# -- Run test with code coverage:
$ coverage run -m pytest
# -- AFTERWARDS:
$ coverage report
... REVIEW NOTE @jsa34:
|
NOTE: The |
I think this is good to go now. Any objections if we merge it? We still need a GA workflow for releasing the Python code, but I think that should be done separately from this PR. |
@aslakhellesoy @jsa34
NOTES:
|
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.
Great work!! Just waiting for @jenisys' final changes/feedback now.
@jsa34 , Sorry I have been AFK
This is really very nice work!!OK, it took me an hour to find something I could contribute... Pathlib: because paths are not stringsSince @aslakhellesoy mentioned Windows users, consider using Python can now join the READMEConsider extending the list of examples in the README to include Python. |
use poetry to manage dependencies and as a build tool preparing for release
I have responded to @blaisep 's feedback. I already had updated the README.md entry (I hope!) - thanks, @blaisep ! @jenisys - good points! Updated for pylint comment: I sacrificed a type hint to avoid circular dependency hell! I had moved the import to be inline to avoid this, but better to be safe than sorry! I hadn't considered the build part in this scope, and have never had to release anything, but just learned about Poetry and seemed like a great idea to use (learning so much doing this!) I have updated this to use poetry and so if this accepted I believe adding the deployment script should be trivial, but never released anything on Pypi so thought I would hold fire going that far :) |
@aslakhellesoy @jsa34 NECESSARY BEFORE THE MERGE (in my opinion):
DETAILS:
|
@jsa34 ,
I agree with Jens about the p3 naming because it is nicer to say: (I think there is also a namespace restriction of hyphens in module names) |
Hello! Thank you once again for great feedback. I have address what I can, but I agree the documentation needs to be looked properly, which would be good to come back to with a follow up MR as I don't have capacity at the moment. @jenisys : I have addressed points 1-3 (thank you!) I have added a release yml (provisionally), but it obviously needs secrets set up. Any other feedback, as always, much appreciated! |
Hi @jsa34 , my schedule is more open this week, and I would be very happy to help with the documentation as well as to borrow some of the cleverness @jenisys does with It may be easier to do this realtime. You can reach me at home: blaise at gmail dot com
|
Is anything holding this PR up? If there are remaining issues to be fixed, can we do that in a new PR? |
AFAIK: no.
Any remaining things can be done afterwards (if necessary).
|
Description
Cucumber expressions for Python
Motivation & context
Fixes #27
Type of change
Checklist: