Skip to content
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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

evelyn9191
Copy link
Contributor

@evelyn9191 evelyn9191 commented Feb 1, 2020

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.

@evelyn9191 evelyn9191 force-pushed the add-pre-commit-configuration branch 24 times, most recently from 60c613c to 63f1433 Compare February 3, 2020 19:11
@evelyn9191 evelyn9191 changed the title WIP: Add pre commit configuration Add pre commit configuration Feb 8, 2020
@@ -2,7 +2,7 @@ language: python
python:
- "3.8"

install: "pip install . pre-commit"
install: "pip install Cython && pip install . pre-commit"
Copy link
Member

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

Copy link
Contributor Author

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.

@rkdarst
Copy link
Member

rkdarst commented Mar 2, 2020

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.

@evelyn9191
Copy link
Contributor Author

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.

@evelyn9191 evelyn9191 changed the title Add pre commit configuration Add pre commit configuration (original, reworking) Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants