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

Change code style to be in line with PEP8 format #4

Closed
wants to merge 1 commit into from

Conversation

vanderhe
Copy link
Contributor

@vanderhe vanderhe commented Aug 2, 2022

Fixes installation via pip and formats according to PEP8 using Black. For the future we might think about integrating the formatting step into a CI workflow, but for now I just ran Black manually.

@vanderhe
Copy link
Contributor Author

vanderhe commented Nov 7, 2022

I will resolve the conflicts a.s.a.p.

@ThijsSmolders
Copy link
Collaborator

Hi Tammo! I don't mind updating this either. I have done some work on implementing a CI workflow, I actually might as well try to add black to it, not a bad idea at all!

@vanderhe
Copy link
Contributor Author

vanderhe commented Nov 7, 2022

@ThijsSmolders As it suits you better. I have mixed feelings about integrating Black formatting into the CI workflow. I can spontaneously imagine different scenarios:

  1. on-the-fly reformatting while merging
    disadvantage: the code that enters the repository is not the code that was contributed
  2. enforcing formatting by comparing the formatted version with contributed code, therefore block merging if there is a difference
    disadvantage: additional effort for developers
  3. reformatting as additional (automated) commit to PR
    open questions: to squash or not to squash, ...

@ThijsSmolders
Copy link
Collaborator

@vanderhe Thanks for your input Tammo! I think they're valid concerns, but my initial thoughts are that it's probably still worth implementing. In terms of your first point, there's already a step in the CI (which I think is useful) that bumps the version based on the commit messages. As such, the source code on the repo is ahead of the local branch after a push either way (though this is limited to only the CHANGELOG.md and pyproject.toml files that keep track of the version number). This step already makes git pulls and fetches more important, making the additional importing of formatting corrections no additional effort. As to your second point, you're right in saying that merge conflicts can be a real pain. I guess however that PEP8 formatting is a good rule to abide by anyway, so implementing it is not a bad idea. I guess one should always try and write keeping PEP8 formatting in mind, and ideally run black locally before pushing such that the formatted version (using black in the CI) is always the same as the contributed code. Not doing either of the first two steps locally should come perhaps come with a penalty that disincentivises this behaviour in the form of a merge conflict to be resolved haha!

@vanderhe vanderhe closed this Nov 15, 2022
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.

2 participants