Develop#6
Conversation
Create python-package.yml
add python3 support
Merge Forks
Bug Fixes and improvements
russell
left a comment
russell
left a comment
There was a problem hiding this comment.
I started reviewing this PR yesterday, but only got part way through it before you closed it
russell
left a comment
russell
left a comment
There was a problem hiding this comment.
everything looks fine from what i can see, it's mostly just type additions, Great stuff! I'll have a play locally soon
|
sorry for closing the pull request :-( |
|
Hey folks - the possibility of Python 3 support is great. Do you have an idea of when you would plan to release a version with that included? |
|
@tomduckering I have made a release for my own projects, because I'm waiting for a review and a new release. https://pypi.org/project/sifter3/ @garyp @russell Is it ok for you, when I'm continue development in my fork and create new releases as sifter3? |
|
the last comment from Garry seemed to indicate he was going to transfer the repo to the org. Lets just double check if he can do that, if not we can always rename it to sifter3 like you mention. But it would be nice if we can continue under the same package name if possible. My guess is that without using the @ mentions he won't get notifications, and Garry is focused on other things :) With the current state of this branch, I would say this should become the new master, I can then rebase my minor changes onto it. Thank you @manfred-kaiser for your great work |
|
@russell I have already included some of the commands and tests from your fork in my version of sifter. Included from @russell fork: Included in my version:
Not included:
I will not include "pipe" and "rewrite" in the base package of sifter, because there is no RFC, but there is the possibility to create an unofficial/custom extensions package. New commands
|
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| - uses: actions/cache@v2 |
There was a problem hiding this comment.
Is there a reason to stop caching dependencies?
There was a problem hiding this comment.
I have switched to the default github action for testing python packages, This is the reason, why this was changed
There was a problem hiding this comment.
The default Python GitHub Action provides a good starting template for a Python project. In my experience on other projects, that template usually needs to be modified/improved for real-world projects. Even the main GitHub docs provide examples of useful changes to the default template. For example, here's their suggested change to support caching: https://docs.github.com/en/free-pro-team@latest/actions/guides/building-and-testing-python#caching-dependencies.
Looking at the current builds in GitHub, they run fast enough. So we don't necessarily need caching of the Python dependencies. But having caching in there doesn't hurt either, so I was surprised to see you take it out.
| "sifter.t" : ["*.in", "*.out", "*.msg", "*.rules"], | ||
| }, | ||
| install_requires=[ | ||
| "ply" |
There was a problem hiding this comment.
Don't we still need the dependency to be in here? I haven't been using Python as much recently, so I'm not sure if conventions have changed. But usually projects put their dependencies into install_requires so that they'll be properly included in the pip package info.
There was a problem hiding this comment.
Those files are only needed by unit tests. unit tests are not included in the package. So there is no need to include the files.
We can include unit tests in the source distribution, but I would recommend to keep the releases free from development stuff.
There was a problem hiding this comment.
I agree with you on the files related to unit tests. But that's not what I was commenting on. I was asking whether install_requires=["ply"] should still be included?
| - name: Test with pytest | ||
| run: | | ||
| tox -e py | ||
| pytest |
There was a problem hiding this comment.
Is there a reason to get rid of tox? I find tox helpful because it makes it easier to run the same tests locally (including across multiple python versions and including the flake8 linting) that are being run by CI.
There was a problem hiding this comment.
As mentioned before, I have switched to the default github action for testing python packages, We can include a tox file, if you want to use tox for testing.
There was a problem hiding this comment.
As I mentioned, I think tox really helps with being able to run the tests locally without having to push it to GitHub first. Given that the work has already been done to enable tox in this project, I'd prefer to not rip it out.
There was a problem hiding this comment.
To be clear: I'm good with the switch to using pytest. Just make that switch inside the tox.ini file instead of in the GitHub Actions workflow, so that tox can be used to run pytest.
|
development moved to new repository https://github.com/python-sifter/sifter3 |
5 participants
No description provided.