-
Notifications
You must be signed in to change notification settings - Fork 8
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
Some linting fixes #199
Some linting fixes #199
Conversation
I’ve commented the ones that refer to commented out code.
@andylolz On a personal note I am just not looking forward to have to fix all the linting soon so we can merge down to dev so even this is a godsend. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most looks good but I have one question about the strings.py
file 🐇
iati/tests/data/strings.py
Outdated
@@ -4,11 +4,74 @@ | |||
|
|||
MEDIUM_TEXT = '''Lorem ipsum dolor sit amet, nisl quam, odio ut mauris, mi et nullam. Facilisis penatibus a, integer fusce vel, auctor elit. Aliquet viverra nunc, wisi a wisi. Suspendisse praesent, nec nisl, nam scelerisque sociis.''' | |||
|
|||
LONG_TEXT = '''Lorem ipsum dolor sit amet, nulla magna ut elit arcu donec adipiscing, metus nunc vel suscipit posuere risus rutrum, mauris velit ultricies platea mi, integer adipiscing et id lectus rutrum. In aliquam nisl ante vehicula, massa quis quis maecenas vestibulum. Commodo inceptos quis vestibulum elit, et feugiat wisi, quam sollicitudin suscipit eget volutpat condimentum, eu morbi, lacus laoreet suspendisse phasellus. Dolor non mauris aenean curabitur ipsum augue, ante aliquam, auctor et aenean, feugiat odio elit est facilisis etiam, nullam wisi in sed wisi. Nec tortor vel, arcu et congue. Velit risus consectetuer incididunt id arcu, integer at sem vitae volutpat aliquet, arcu wisi non integer lacus sit et, in velit in purus. Dolor sagittis quam magna, at proin neque consectetuer. Et aptent consequat metus ante lectus faucibus, mi eu vulputate dignissim, pede et viverra et morbi sit praesent, velit nulla justo fringilla cras id at, tellus condimentum sit nec. Nulla ullamcorper dictum justo magna nibh arcu, vitae etiam sit fermentum turpis.\nOrci habitasse cras sem, vitae blandit tincidunt pede quam. Morbi elementum egestas id, sollicitudin molestias ultricies orci nonummy pede, tellus nisl ut magna, sed mattis hendrerit eget, pede non enim. Nam cubilia dictumst libero nonummy accumsan, lacus arcu auctor. Condimentum tellus. Mi nulla tincidunt nisl adipiscing elit. Nam et malesuada praesent fringilla amet, suscipit montes, eget libero litora, orci hymenaeos ullamcorper. Vulputate luctus magna eu vivamus, quisque sed tempor eget ipsum ut, in justo molestie ligula massa, pharetra duis. Congue elit ultricies, rutrum sapien vitae, eget sed, in lorem. Nulla sit purus etiam lorem rhoncus tempus, aliquam sapien, sed pellentesque facilisis mauris, et at mi consequat ultrices egestas, semper proin massa duis. Vel duis lorem donec elit purus augue. Iaculis lectus mauris fringilla quam ligula, interdum donec ante molestie ultrices mauris mauris.\nTellus scelerisque sit fusce, ac leo vitae nunc sem viverra. Ipsum elementum non pellentesque quam, amet libero. Parturient mus mauris in donec ad, eros dictum leo felis, lectus wisi praesent enim semper iaculis consequat, at sollicitudin nunc sed odit, nibh elementum convallis. Sed cursus. Nulla risus, sem vel. Pulvinar sit vestibulum, ac urna eleifend morbi nec pulvinar, morbi cras aliquam molestie, ut quis pharetra mi lacus ullamcorper, condimentum scelerisque ullamcorper orci. Quis faucibus fusce aliquam arcu proin aliquet, eget nec dolor quis at quam, tellus nam vel bibendum erat facilisi. Massa tellus, duis dolores, dui magna etiam integer dolor interdum, suscipit fringilla sit id risus quis tellus. Vivamus ac, lorem auctor iaculis id ultrices lacus, dictum diam nonummy nec mi dignissim. Volutpat nisl metus felis, risus tempus enim lacus sed sapien ac, tristique sed sed cras, consequat dolor lectus vitae lorem torquent, quis urna sociis arcu class.\nSit wisi in, a praesent eu mauris aliquam purus pellentesque, tellus egestas sem egestas vivamus a, elementum lorem a. Leo eget et risus. Malesuada vivamus sociis vestibulum orci, fames urna, ipsum felis aliquam. Integer cursus vel sed, pellentesque malesuada tincidunt nullam. Adipiscing venenatis netus auctor semper sem.''' | |||
LONG_TEXT = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andylolz Everything else looks fine but could I get your thoughts on the changes to this file in particular? In team we try to avoid mid-sentence hard-wrapping where possible in favour of locally soft-wrapping text and excluding deliberately long strings from linting on a case by case basis. Interested to get your views on why you've chosen this style here as it doesn't upset my local linters so I'm unaware of any potential benefits of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing, and for catching this! It was the bit I was least happy with, too.
So essentially I had no idea how to fix this nicely, and this was the best I could manage. I agree that it’s bad / ugly.
You can see on travis that it’s flake8 complaining about this:
https://travis-ci.org/IATI/preview-website/jobs/401877458
iati/tests/data/strings.py:7:256: E501 line too long (3274 > 255 characters)
iati/tests/data/strings.py:11:256: E501 line too long (800 > 255 characters)
I’m more than happy to revert d1fa7f3, and resolve the errors in whatever is the preferred way (e.g. add a directive to ignore them).
This reverts commit d1fa7f3.
|
||
RAW_HTML = ''' | ||
<p align="justify"> | ||
Lorem ipsum dolor sit amet, accumsan neque lectus ligula mattis netus. A pede posuere. Sem cras torquent, mi lorem augue, risus amet et semper vehicula est. Ipsum vivamus, morbi tincidunt, suspendisse non justo montes, sociis nullam elit metus interdum integer, donec amet nulla consequat praesent. Integer mauris morbi quis sapien, pretium aenean pulvinar diam iaculis eros volutpat, amet turpis non placerat suscipit phasellus lacus, lectus sed sem libero aliquam ligula. Consequat tincidunt vitae proin maecenas habitant lorem, parturient lacinia vehicula a dolor, maecenas lectus interdum arcu lobortis quisque mauris, praesent quis primis, purus aliquam ipsum tristique. Curabitur nec, erat tempora, accumsan et enim consectetuer magnis ligula, justo fermentum lacus imperdiet, eu sed habitasse. | ||
Lorem ipsum dolor sit amet, accumsan neque lectus ligula mattis netus. A pede posuere. Sem cras torquent, mi lorem augue, risus amet et semper vehicula est. Ipsum vivamus, morbi tincidunt, suspendisse non justo montes, sociis nullam elit metus interdum integer, donec amet nulla consequat praesent. Integer mauris morbi quis sapien, pretium aenean pulvinar diam iaculis eros volutpat, amet turpis non placerat suscipit phasellus lacus, lectus sed sem libero aliquam ligula. Consequat tincidunt vitae proin maecenas habitant lorem, parturient lacinia vehicula a dolor, maecenas lectus interdum arcu lobortis quisque mauris, praesent quis primis, purus aliquam ipsum tristique. Curabitur nec, erat tempora, accumsan et enim consectetuer magnis ligula, justo fermentum lacus imperdiet, eu sed habitasse.''' # noqa: E501 | ||
RAW_HTML += ''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allthatilk Okay, I tried fixing the long string errors a different way, by telling flake8 to ignore the error on the two lines in question (seems reasonable to ignore these, right?)
For the second instance, I had to split the string in order to get the directive in the right place. It’s a little ugly but hopefully okay?
If you know of (or can figure out) a better way to do this, that would be ace!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andylolz yep ignoring those is fine. Happy to approve this change as I can't see a better way to do it at the moment and it's a small change if needs be in the future 😃
⚽ 🏆 ⚽ 🏆 ⚽ |
A handful of linting fixes, just as an incremental improvement.