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

PEP8 Formatting #157

Merged
merged 4 commits into from
Jan 17, 2022
Merged

PEP8 Formatting #157

merged 4 commits into from
Jan 17, 2022

Conversation

PabloLec
Copy link
Contributor

@PabloLec PabloLec commented Oct 30, 2021

I just made a test, to see what auto formatting tool could best fit the project.
So, in order to keep docstrings intact we cannot use black. That leaves us with autopep8 and yapf, I guess autopep8 could do the job.

I tested it on __init__.py as a demo because it's a pretty big piece.
Configuration is vanilla except for max line length, I set it to 88 as black does, we could set another value.

Tell me if the file looks good to you @jwlodek, or if something seems odd.

In my opinion the code already looks nicer, more consistent.

@jwlodek
Copy link
Owner

jwlodek commented Dec 29, 2021

This looks good, maybe only suggestion would be bump the line length a bit? Maybe to ~95? I have a preference for longer lines where possible. If you could adjust for that and fix the newly found conflicts I'll approve and merge.

@PabloLec
Copy link
Contributor Author

PabloLec commented Jan 5, 2022

So I tried to increase the line length with autopep8 but it broke some parts, like function parameters were left untouched, even if formatting wasn't right, if line length was <95.
Then I tried to switch to yapf, again some strange behavior like:

def some_func():
    pass

became:

def some_func(
):
    pass

Which is just useless and ugly.

So I came back to black with a line length of 95. black just works without any tweak and conf needed which is great. I can't remember what we said about docstrings but they look unchanged so I guess the powerful and simple black will do the job.
Also, not sure about the current conflict, is it a bug ?

@jwlodek
Copy link
Owner

jwlodek commented Jan 5, 2022

I believe the current conflict is caused by some changes I've made on the branch since this PR was opened, may want to pull those changes and try re-running the formatting?

@PabloLec
Copy link
Contributor Author

PabloLec commented Jan 5, 2022

I copy/pasted the current __init__.py from v0.1.5-develop and reran formatting so it must be the same. I'm not too familiar with conflicts though but take a look at GitHub conflicts summary, it looks like an empty line.

@PabloLec
Copy link
Contributor Author

PabloLec commented Jan 5, 2022

Ok, I just deleted that empty line from GitHub it must be good for merge.
So we can either merge it right now or reformat everything prior.
I think it might be a good idea for me to reformat everything right now, have a good clean codebase and make sure to run black at least before a dev branch is merged into master.

@jwlodek jwlodek merged commit 327f5f2 into jwlodek:v0.1.5-develop Jan 17, 2022
@jwlodek
Copy link
Owner

jwlodek commented Jan 17, 2022

Ok, I think it looks fine to me, maybe would want to replace the default " characters with just ', but that's not a big deal, merged. Could you run the formatting on the other files in the project when you have a chance? Also, will need to make some CI jobs that check if formatting matches our requirements, but that can be tracked as a different issue.

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