Issue 29994568: Noissue - Use flake8's built-in per-file-ignores instead of flake8-per-file-ignores (Closed)
|
Created:
Jan. 31, 2019, 11 p.m. by rhowell Modified:
Feb. 7, 2019, 4:19 a.m. Base URL:
https://hg.adblockplus.org/abpssembly Visibility:
Public. |
DescriptionNoissue - Use flake8's built-in per-file-ignores instead of flake8-per-file-ignores
Repository: https://hg.adblockplus.org/abpssembly
Base revision: 845ae7cabd63
Patch Set 1 #
Total comments: 2
MessagesTotal messages: 10
Hey guys, I had to add some new per-file-ignores, and wanted to make sure we're on the same page. I ignored W504 globally (W504 line break after binary operator). We had discussed ignoring W504 in flake8-eyeo instead (https://issues.adblockplus.org/ticket/7196) but since that's not done yet, I just ignored it here. The other flake8 ignores I added were mostly about upper- or lower-case complaints. The full list is here: https://pastebin.com/JSBwyLMy Oh, I also noticed the 12 warnings, and it seems they're all related to formmail. Since we merged formmail.py and formmail2.py in sitescripts, we no longer get those warnings. It seems abpssembly is forked from sitescripts? If you want, I can look into copying the new formmail stuff to abpssembly, and getting rid of those warnings. Also, I'm not sure why there was a random tab in tox.ini (line 118), but my editor automatically converts them to spaces. I could change it back if we really want, but personally I prefer spaces. Let me know if you see anything else? Thanks! :) Rosie P.S. Tristan - I hope you don't mind reviewing. The log showed you had the majority of recent commits in abpssembly, so I wanted to check in with ya.
Hey Rosie! I wouldn't know why i would mind reviewing. I initially wanted to also have .pytest_cache added to .gitignore, but it looks like that's not necessary, see below. So: LGTM :) https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore#newcode9 .hgignore:9: .pytest_cache Interesting - pytest creates a .gitignore file in this folder automatically :D
Hi Rosie, Thank you for taking care of this. I think we should also ignore W503 -- at least that's what we concluded in that discussion in #python. Also it seems that the change to .hgignore has already been applied before (see below). Cheers, Vasily https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore#newcode9 .hgignore:9: .pytest_cache It seems that this line is already there, although with a slash at the end. Added by this commit: https://hg.adblockplus.org/sitescripts/rev/656fee0f59be
> https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore#newcode9 > .hgignore:9: .pytest_cache > It seems that this line is already there, although with a slash at the end. > Added by this commit: https://hg.adblockplus.org/sitescripts/rev/656fee0f59be Ah, now I understand, this is abpssembly and not sitescripts. Never mind this point then. P.S. I usually specify the repository and base revision in review description (like here: https://codereview.adblockplus.org/29992563/) to avoid confusions like this -- I think it's a nice practice.
On 2019/02/01 15:13:14, Vasily Kuznetsov wrote: > > https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore#newcode9 > > .hgignore:9: .pytest_cache > > It seems that this line is already there, although with a slash at the end. > > Added by this commit: https://hg.adblockplus.org/sitescripts/rev/656fee0f59be > > Ah, now I understand, this is abpssembly and not sitescripts. Never mind this > point then. > > P.S. I usually specify the repository and base revision in review description > (like here: https://codereview.adblockplus.org/29992563/) to avoid confusions > like this -- I think it's a nice practice. Ah, sorry for the confusion! I added the repo and base revision to the description. I could also add W503, but there are 2 reasons why I didn't: 1. It wasn't necessary for this change and 2. I plan on picking up #7196 soon, so both W503 and W504 will be ignored in flake8-eyeo, and the ignores here will be irrelevant anyway. (And I guess then W503 & W504 can be removed from the tox.ini files too). I'm happy to add it, though, if you think it would be better?
On 2019/02/02 02:38:04, rhowell wrote: > On 2019/02/01 15:13:14, Vasily Kuznetsov wrote: > > > https://codereview.adblockplus.org/29994568/diff/29994569/.hgignore#newcode9 > > > .hgignore:9: .pytest_cache > > > It seems that this line is already there, although with a slash at the end. > > > Added by this commit: > https://hg.adblockplus.org/sitescripts/rev/656fee0f59be > > > > Ah, now I understand, this is abpssembly and not sitescripts. Never mind this > > point then. > > > > P.S. I usually specify the repository and base revision in review description > > (like here: https://codereview.adblockplus.org/29992563/) to avoid confusions > > like this -- I think it's a nice practice. > > Ah, sorry for the confusion! I added the repo and base revision to the > description. > > I could also add W503, but there are 2 reasons why I didn't: > 1. It wasn't necessary for this change and > 2. I plan on picking up #7196 soon, so both W503 and W504 will be ignored in > flake8-eyeo, and the ignores here will be irrelevant anyway. (And I guess then > W503 & W504 can be removed from the tox.ini files too). > > I'm happy to add it, though, if you think it would be better? I think having both errors ignored would be more "correct" but indeed it seems like it doesn't matter for the time being and maybe ever. I'm ok with leaving W503 out, especially if Tristan also thinks so. Cheers, Vasily
On 2019/02/04 13:54:21, Vasily Kuznetsov wrote: > I think having both errors ignored would be more "correct" but indeed it seems > like it doesn't matter for the time being and maybe ever. > > I'm ok with leaving W503 out, especially if Tristan also thinks so. > > Cheers, > Vasily Still LGTM ;)
Ah, yeah, LGTM from me too!
On 2019/02/04 14:48:23, Vasily Kuznetsov wrote: > Ah, yeah, LGTM from me too! Great, thanks! It seems I don't have push access to this repo. Could someone push this for me? Or should I request access from ops? 
|
