search

justinabrahms / imhotep

A static-analysis bot for Github
MIT License
223 stars 42 forks source link

File-level Violations #87

Closed danpalmer closed 8 years ago

danpalmer commented 8 years ago

This PR adds optional reporting for linting violations at the file level, addressing issue #85.

A concrete example of this is flake8-isort, which reports a file not being “isorted” as having a violation on line 0.

This change adds the option --report-file-violations, which will cause the value 0 to be forcibly added to the ‘changed lines’, so that Imhotep will include violations on line 0. This behaviour is then documented in tools.py. Finally, the mapping of line numbers to diff positions is updated to include a mapping form 0 to the lowest value line in the diff, so that Imhotep will report file violations on the first line it can in a given file.

Comments and thoughts on implementation/documentation/etc welcomed.

justinabrahms commented 8 years ago

Hey Dan.

I'm out of town at the moment and don't have the mental bandwidth to give you a review. The overloading 0 thing seems non-optimal. I'm wondering if tools couldn't just return a different entry object entirely which would denote a different sort of comment?

This dovetails with a change I made recently to allow posting on the entity itself (pr or commit), but wasn't able to implement it w/ a nice interface b/c the call signatures wouldn't match across reporters. Part of this has me thinking that I should introduce some sort of parameter object.

Not well formed thoughts on this right now, but wanted to give you some feedback.

danpalmer commented 8 years ago

Hi Justin. Thought I'd try and resurrect this – I'm trying to expand our usage of Imhotep (possibly writing some additional tools).

I had 2 reasons for overloading the 0 value: it was the simplest code change to make since ultimately it needs to be posted onto a file at a particular line anyway for GitHub, and flake8-isort (my original use-case) reports issues as being on line 0, which means that this would 'just work'.

I suppose we could move the line 0 overloading down into the flake8 tool, add a new return value to the interface for file-level violations, and then also move the special case up into the GitHub integration. This way the core has a more explicit API, and it's up to each side of that to implement special cases as needed.

What do you think? Do you have any alternative ideas? I don't think the current implementation is that bad though so it's up t you if you think we need to refactor this.

justinabrahms commented 8 years ago

Thanks for following up, Dan. I think the 0 line number thing is good enough to unblock you. Ultimately, I think having cleaner semantics would be nice, but there's little harm in overloading 0, given you can't have a line number 0. Thanks so much!

danpalmer commented 8 years ago

Thanks for the merge, much appreciated, sorry for taking so long to pick this up again.