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

Cosmetic change to adapt to new pep8 style #99

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

elfosardo
Copy link
Collaborator

First of a series of cosmetic changes to migrate to new pep8 style.
The main change is cutting the lines to less than 80 characters.
More style changes may be applied depending on the module.

This patch does not include any functional change.

First of a series of cosmetic changes to migrate to new pep8 style.
The main change is cutting the lines to less than 80 characters.
More style changes may be applied depending on the module.

This patch does not include any functional change.
@elfosardo elfosardo mentioned this pull request Jul 21, 2019
@dtantsur
Copy link
Collaborator

Could you make the CI gate on this? Otherwise it's useless.

@elfosardo
Copy link
Collaborator Author

I could, but I think that would mean change tox.ini and so applying the change to the entire code at the same time.
No problem for me, it's basically an automated change, it just becomes a bit bigger to review.

@dtantsur
Copy link
Collaborator

How big? I'm inclined to do it in one shot, unless it's really crazy (hundreds of changes).

@elfosardo
Copy link
Collaborator Author

If we consider only the line length changes, definitely not hundreds.
Some changes are also fixed by #95 with the move of all the strings to a different file, so that should be merged first anyway; smart_utils_results.py needs the same treatment for the migration to constants, little impact anyway and I can add it to #94 and wait for the merge.
If we add the docstrings changes for correct format (it's correcting ' to " and add parameters and other stuff), then we go up in numbers, but we can exclude them at the beginning and add proper docstrings module by module in future patches.

@tbreeds
Copy link
Collaborator

tbreeds commented Jul 22, 2019

Looks good to me.

Once we complete all the changes we've talked about in #84 I'm totally in favor of enforcing pep8 in travis

@elfosardo
Copy link
Collaborator Author

@dtantsur @tbreeds please let me know if there is anything more to do to be able to move forward with this

Copy link
Collaborator

@dtantsur dtantsur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do enforce it in the CI soon

@dtantsur dtantsur merged commit 80922f0 into redhat-cip:master Aug 6, 2019
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.

3 participants