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

Black and iSort Code Formatting #488

Closed
danielsclint opened this issue Sep 23, 2021 · 6 comments
Closed

Black and iSort Code Formatting #488

danielsclint opened this issue Sep 23, 2021 · 6 comments
Labels
Style Formatting changes to code

Comments

@danielsclint
Copy link

As we have more contributors to the code, we should begin to enforce code formatting. The review of the SANDAG cross-border model and other enhancements revealed code changes that added line breaks and other formatting changes. Code format is inherently biased by the person writing the code, and we could end up with lots of formatting changes in the Github repo rather than substantive changes over the long term.

This is a low priority request, but it is also super easy to implement.

Black Project Home
iSort Project Home

@jpn--
Copy link
Member

jpn-- commented Sep 23, 2021

We do have some basic code formatting enforced currently by pycodestyle, it's not like it's the wild west out here currently. I'm not fundamentally opposed to black instead, but suggesting it's "easy" is leaving out a lot of work for anyone who's working on code updates that don't get merged first. This is a huge project already with at least 57 known forks, and doing this is like dropping a glitter bomb on anyone working on modified versions of the code -- it shouldn't actually break anything but it'll make a huge mess to clean up.

@e-lo
Copy link
Collaborator

e-lo commented Sep 23, 2021

I like this as it makes sure that future diffs are really...diffs. I suggest the following:

  • add black as a pre-commit hook in main and develop
  • add a commit that is purely a "cleanup" commit with a message reminding PRs to run black
  • when developers update their forks to pull back to develop (which they should do), they will also add the pre-commit hook which will run on any updates to their PR.

Just my 2cents

@danielsclint
Copy link
Author

I like @e-lo suggestion of a pre-commit hook. I'm not necessarily wed to Black, but I do feel like we need a format enforcer like Black or AutoPep8. It eliminates any subjected, personal formatting bias (in addition to ensuring diffs are diffs). Removing that subjective bias is great, because I don't ever have to call-out someone for using a tab instead of 4 spaces and other nitpicky formatting issues. I'm sure my syntax is always awesome, and it never annoys anyone though. :)

This is a good article on how to set it all up using the pre-commit hooks.

@bstabler
Copy link
Contributor

bstabler commented Oct 7, 2021

We agreed on the 10/7 call to try this after all the open PRs are merged and we're about to release the next version since this will be the least inconvenient moment.

@bstabler bstabler self-assigned this Oct 7, 2021
@bstabler bstabler removed their assignment Nov 10, 2021
@jpn--
Copy link
Member

jpn-- commented Nov 19, 2021

I've discovered an issue with pre-commit that makes it unappealing for use on Windows when also using conda. pre-commit/pre-commit#1329

@jfdman jfdman added the Style Formatting changes to code label Nov 19, 2021
@xhochy
Copy link

xhochy commented Dec 30, 2021

I've discovered an issue with pre-commit that makes it unappealing for use on Windows when also using conda. pre-commit/pre-commit#1329

Should be fixed now 🥳

@jpn-- jpn-- mentioned this issue Aug 3, 2022
@jpn-- jpn-- closed this as completed Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Style Formatting changes to code
Projects
None yet
Development

No branches or pull requests

6 participants