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

Fix a few lint issues. #90

Merged
merged 3 commits into from
May 8, 2018
Merged

Conversation

jmchilton
Copy link
Member

No description provided.

setup.cfg Outdated
@@ -13,5 +13,6 @@ logging-level=INFO
[flake8]
max-line-length = 150
max-complexity = 30
ignore = E501
Copy link
Contributor

@rhpvorderman rhpvorderman May 8, 2018

Choose a reason for hiding this comment

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

Why ignore line too long? Or does this interfere with max-line-length?
I prefer there to be at least some check on line length. I can't see why lines >150 characters might be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

We ignore this on most Galaxy project. I'm just wondering why we get more lint errors it we ignore E501. Before I added this it was only E501 error, now its much more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why ignore it, and not fix the line length instead?

Copy link
Member

Choose a reason for hiding this comment

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

Because we ignore this pep8 rule in most of the Galaxy projects. I think there is an agreement to not follow it.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion it is better to strictly enforce style (including line length) in projects that have multiple users collaborating, because this improves readability. But in the end this is not my decision to make. My concern is that not strictly enforcing style will lead to maintainability issues down the road.

Copy link
Member

Choose a reason for hiding this comment

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

Everyone agree on this. But a lot of people do not follow this specific rule as we all have big monitors :)
https://github.com/search?q=ignore+%3D+E501&type=Code

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like Pulsar and Planemo enforce the max line length of 150 also, I think agree that should be long enough. I'd prefer just to fix the long lines. I thought I put this in there because it was enforcing 80 for some reason which is in my opinion unworkable when I've tried it in the past. Mind if I close this PR @bgruening, sorry I was confused.

Copy link
Member

Choose a reason for hiding this comment

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

please go on ...

setup.cfg Outdated
@@ -13,5 +13,6 @@ logging-level=INFO
[flake8]
max-line-length = 150
max-complexity = 30
ignore = E501
Copy link
Member

@drosofff drosofff May 8, 2018

Choose a reason for hiding this comment

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

@jmchilton Please feel free to remove

                                   "Christophe Antoniewski <drosofff@gmail.com>\n"
                                   "https://github.com/ARTbio/ansible-artimed/tree/master/extra-files/generate_tool_list_from_ga_workflow_files.py")

from generate_tool_list_from_ga_workflow_files.py. It is non sense as well as deprecated.

@jmchilton jmchilton merged commit 6bc39d8 into galaxyproject:master May 8, 2018
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