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

[cling-cpt] Split long lines to get rid of E501 error [skip-ci] #11010

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

saisoma123
Copy link
Contributor

This Pull request: Makes the code more flake8 compliant.

Changes or fixes: Split lines that were longer than 79 characters.

Checklist:

  • tested changes locally
  • [NA] updated the docs (if necessary)

This PR fixes issue #406 mentioned in (root-project/cling#406)

@saisoma123 saisoma123 requested a review from vgvassilev as a code owner July 20, 2022 23:24
@phsft-bot
Copy link

Can one of the admins verify this patch?

@saisoma123
Copy link
Contributor Author

Will make more pull requests to fully get rid of the E501 error

Comment on lines 1926 to 1927
"sudo {0} -m pip" +
" install distro".format(sys.executable), shell=True
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to split at the , to avoid breaking the string literal (same below, where it applies).

Comment on lines 2310 to 2312
#download_llvm_binary()
#compile = compile_for_binary
#install_prefix = install_prefix_for_binary
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this additional change? It's not part of the PR description.

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM! I am surprised they only report the issues but to not provide an automatic generate a fix for these coding violations.

@vgvassilev
Copy link
Member

@saisoma123, can you rebase this PR?

@saisoma123
Copy link
Contributor Author

@vgvassilev Yeah I think there is a way for automatic linting I'll do that instead. I'll also rebase the PR right now

@saisoma123
Copy link
Contributor Author

Yeah just rebased the PR

@vgvassilev vgvassilev merged commit 305f695 into root-project:master Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants