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

Formatting: fix long lines #345

Merged
merged 1 commit into from
Jul 17, 2019
Merged

Conversation

Glutexo
Copy link
Collaborator

@Glutexo Glutexo commented Jul 11, 2019

Fixed the E501 Flake8 rule targetting too long lines. In most places, the code has been only reformatted. Sometimes a variable name has been shortened or another code-aware change has been made. I am not sure whether these changes don’t trigger another Flake8 rule. That should not be a big of an issue, since such cases should be rare and can be easily fixed.

I used a line length of 88 character, which is what Black uses by default.

In the app.config module I changed the import statement and replaced os.environ.get with an equivalent os.getenv.

In the api.metrics module I was targeting only the long lines, which results in not-unified code style. Improving the style and making it consistent would be a subject of another pull request.

This is a part of the first step towards a nicely formatted codebase. See #189, #335 and #331. It is currently the biggest pull request, but containing only trivial changes not affecting logic.

@Glutexo Glutexo self-assigned this Jul 11, 2019
@Glutexo Glutexo changed the title Fix E501 rule Formatting: fix long lines Jul 11, 2019
Copy link
Contributor

@catastrophe-brandon catastrophe-brandon left a comment

Choose a reason for hiding this comment

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

If memory serves, I think we chose to allow 100-char lines in the config for the test code. Personally, I think the 80 column limit is pretty restrictive. As far as I'm concerned, pick whatever line limit you like as long as we consistently enforce it.

Edit: Confirmed that we use a 100-char limit in our setup.cfg:

[flake8]
ignore = E128,E811,E999,W504
max-line-length = 100

@Glutexo
Copy link
Collaborator Author

Glutexo commented Jul 15, 2019

Oh, thanks for clarifying. I was not sure whether @diegorafrh didn’t pick 100 just arbitrarily. It would be nice to document this in the README.

@Glutexo
Copy link
Collaborator Author

Glutexo commented Jul 15, 2019

Extending the line length to 100 reduced the number of violating lines. Updated this pull request to use this value instead.

@diegorafrh
Copy link
Contributor

@catastrophe-brandon is right, I did follow what IQE have implemented but I also thought a little bit more about it: #333 (review)

Changed the maximum line length to 119 characters. Fixed the E501 Flake8
rule targetting too long lines. There is only one line that is too long
for this limit.
@Glutexo
Copy link
Collaborator Author

Glutexo commented Jul 17, 2019

Based on @catastrophe-brandon ’s comment I increased the maximum line length to 119 characters. With the limit being this high, there was only one line that violates the rule. Moreover this line was formatted like that probably by mistake, because it was a really long chopped into pieces, separated by spaces "like " "this", but kept on a single line.

@dehort dehort merged commit 815933a into RedHatInsights:master Jul 17, 2019
@Glutexo Glutexo deleted the flake8_long_lines branch July 18, 2019 12:35
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.

4 participants