-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add pre commit configuration (original, reworking) #28
base: master
Are you sure you want to change the base?
Add pre commit configuration (original, reworking) #28
Conversation
60c613c
to
63f1433
Compare
79fd8c1
to
2b5803c
Compare
@@ -2,7 +2,7 @@ language: python | |||
python: | |||
- "3.8" | |||
|
|||
install: "pip install . pre-commit" | |||
install: "pip install Cython && pip install . pre-commit" |
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.
Could this be because install_requires happens only at install time, but cython basically needs to be a dev requirement? Should we make a requirements-dev.txt file which has cython and nose in it? That's what I have done in other projects. Then the first half becomes pip install -r requirements-dev.txt
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.
Applied in the new PR.
Historically I haven't been a big fan of automatic code formatting, because there are useful context cluses for me as a developer in the formatting - especially vertical space and patterns in comments. But, if someone does good work, we should use it. I guess this might conflict with some of the other things that will come soon. Is it easy to split this into different PRs that do each different component? I see a lot of commits here, if each is manual work, we can try to do it as-is. I just need some time to review it all. |
Some are manual work, some not. I agree it is a large PR and it should be split. I have taken out the first part where the basic pre-commit config and Cython stuff is solved (including your comment above) and Black did the automatic formatting. See here. I would keep this PR open, though, and depending on how the splitting would go due to the amount of the manual work and the new changes that may be merged in the meantime to master, decide whether to rebase and adjust this one or create new ones entirely. |
The functionality of this library is awesome. Let's make the code awesome, too.
It is difficult to do changes in the current code. With new and new contributors, it could become even more difficult. I believe that following some basic Python developing good practices can help us to make the code more clean.
I fixed the formatting of the current code as much as I could and added linting checks to make us write cleaner code.
Besides the commit changing the formatting as suggested by
Black
, other commits should be reviewed with regards to whether or not the change will influence the functionality of the library.I also needed to change a bit the installation of Cython. Btw, before, there were errors thrown during the libraries installations that the
gtfspy
couldn't be built/couldn't get its wheels or similar. Yet, the build continued happily and the tests passed. If you know why it didn't fail although an error was raised, it would be great if you could share the reason.