Skip to content

Add flake8 problem matcher#3714

Merged
jackwilsdon merged 2 commits intobeetbox:masterfrom
jackwilsdon:problem-matcher
Aug 2, 2020
Merged

Add flake8 problem matcher#3714
jackwilsdon merged 2 commits intobeetbox:masterfrom
jackwilsdon:problem-matcher

Conversation

@jackwilsdon
Copy link
Copy Markdown
Member

Description

This PR implements the flake8 half of #3614 using a problem matcher, giving us nice inline errors like this on PRs:

inline error

I've had to disable PY_COLORS here, as the ANSI escape sequences were tripping up the regex used in the matcher.

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

@jackwilsdon
Copy link
Copy Markdown
Member Author

jackwilsdon commented Aug 2, 2020

Hmm, I've noticed that even though we're setting the code group, GitHub isn't actually showing the error code anywhere. Is it worth us compounding this back into the message and dropping the code group completely? We'd end up with the code in the message like this:

inline error

@jtpavlock
Copy link
Copy Markdown
Contributor

jtpavlock commented Aug 2, 2020

I just want to double check this won't be effected by the changes in #3694. In particular, the output from pre-commit run -a would display flake8 errors as such:

flake8...................................................................Failed
- hook id: flake8
- exit code: 1

beetsplug/chroma.py:1:1: FI10 __future__ import "division" missing
beetsplug/chroma.py:1:1: FI11 __future__ import "absolute_import" missing
beetsplug/chroma.py:1:1: FI13 __future__ import "print_function" missing

@jackwilsdon
Copy link
Copy Markdown
Member Author

Provided we output ::add-matcher::.github/flake8-problem-matcher.json in one of our steps, it should continue to work with flake8 output no matter where it is:

image

@jtpavlock
Copy link
Copy Markdown
Contributor

Hmm, I've noticed that even though we're setting the code group, GitHub isn't actually showing the error code anywhere. Is it worth us compounding this back into the message and dropping the code group completely? We'd end up with the code in the message like this:

I like it with the error code, but probably not worth it if it's too much trouble

@jackwilsdon
Copy link
Copy Markdown
Member Author

jackwilsdon commented Aug 2, 2020

Changed in 5d24cb0 - was a nice and easy fix 👍

@jtpavlock
Copy link
Copy Markdown
Contributor

will anything change if you do [^\s] rather than [^ ] ? (sorry for not asking this before you changed it 😄 )

@jackwilsdon
Copy link
Copy Markdown
Member Author

I don't think so — the match is working correctly but GitHub just doesn't display the code anywhere in their UI.

I'll give it a shot in a bit though to see if anything interesting happens.

@jtpavlock
Copy link
Copy Markdown
Contributor

the match is working correctly but GitHub just doesn't display the code anywhere in their UI.

Ah okay, I see what you're saying now. Well this looks cool! I'm excited to see it in action. I think you should feel free to merge when you're ready 👍

@jackwilsdon
Copy link
Copy Markdown
Member Author

Awesome, thanks!

@jackwilsdon jackwilsdon merged commit 78d8e31 into beetbox:master Aug 2, 2020
@sampsyo
Copy link
Copy Markdown
Member

sampsyo commented Aug 3, 2020

Wow, this is awesome! I didn't know this was possible. This will be such a time saver!!

@jef
Copy link
Copy Markdown
Member

jef commented Aug 6, 2020

Wow... This was a lot easier than expected. Nice work guys.

@jackwilsdon
Copy link
Copy Markdown
Member Author

There may be some beneifts to using custom actions but this seemed like the path of least resistance for the time being.

@jef
Copy link
Copy Markdown
Member

jef commented Aug 6, 2020

That makes sense to me. Much easier just to put in this way rather than wrap it up another action.

We could've used local actions, but IMO that would be even worse (readability) I suppose.

Nice work!

@jackwilsdon jackwilsdon deleted the problem-matcher branch June 19, 2023 15:23
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