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

Make the entire code base PEP8 compliant. #329

Closed
carldlaird opened this issue Jan 30, 2018 · 11 comments · Fixed by #2800
Closed

Make the entire code base PEP8 compliant. #329

carldlaird opened this issue Jan 30, 2018 · 11 comments · Fixed by #2800

Comments

@carldlaird
Copy link
Member

No description provided.

@mrmundt
Copy link
Contributor

mrmundt commented Jan 27, 2023

IDAES recently implemented the usage of black to enforce PEP8 compliance; in interest of compatibility, I would like to implement the same. I intend to run black on the entire codebase after the following are merged:

@jsiirola
Copy link
Member

I have one more big PR coming that it would be nice to wait a bit and not conflict. I am still working to get a couple tests passing and then can put up a WIP PR so that you can (hopefully) avoid those files while you start applying black.

Should we do black in chunks to keep the PR process simpler, or do it in one big chunk so that we only need to have a small number of revs in the .git-blame-ignore-revs file (see here)

@mrmundt
Copy link
Contributor

mrmundt commented Jan 27, 2023

I intend to do the black application in chunks so as to not make it a reviewing nightmare.

Also, for posterity, I intend to run this command: black -S <path> <- The -S skips string normalization.

@blnicho
Copy link
Member

blnicho commented Feb 10, 2023

Double check these files to make sure the line numbers are correct after applying black

The problematic line was replaced in #2754

@bernalde
Copy link
Contributor

Will black also affect contrib directories?

@blnicho
Copy link
Member

blnicho commented Feb 10, 2023

@bernalde yes but we're applying it in phases to reduce merge conflicts as much as possible with currently open PRs (including the MindtPy rewrite). If you have a specific module in contrib that you want us to wait to apply it to let us know.

@bernalde
Copy link
Contributor

None in particular right now. I would say that for those of us writing code, it might be better if we are told how to implement the formatting correctly. Something along the lines of https://jump.dev/JuMP.jl/stable/developers/style/#Style-guide

@blnicho
Copy link
Member

blnicho commented Feb 10, 2023

Yep, we will definitely document formatting expectations after finishing this first pass. I expect we'll also need some automated infrastructure to make sure new PRs are complying but we haven't discussed what this will look like yet.

@mrmundt
Copy link
Contributor

mrmundt commented Feb 10, 2023

I have a plan to implement something similar to what IDAES does in that it has a linting check at the beginning of the job pipeline - if the linting is wrong, none of the other jobs will run.

@mrmundt
Copy link
Contributor

mrmundt commented Feb 14, 2023

Adding another comment for posterity:

  • Sometimes black will combine previously wrapped lines of string and not get rid of the intermediary quotation marks. These need to be removed so they aren't quite as messy.

@mrmundt
Copy link
Contributor

mrmundt commented Apr 10, 2023

For closure:

  • The command we run in its entirety is black -S -C <path> --exclude examples/pyomobook/python-ch/BadIndent.py (we ignore that one file as it is intentionally wrong)
  • The codebase was updated using black 23.3.0
  • Sometimes line-wraps can happen such that it leaves " ", ' ', or r" " in the middle of a string. These were fixed manually.
  • Also implemented as part of this was a spell checker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants