Hi @justinabrahms.

I've just changed the supported Python versions (to drop 2.6 and add 3.5/3.6, I hope that's not too bold). I've also restructured the travis build matrix to support that, and merged an outstanding bugfix PR that I had where the tests had been failing due to the Python build issues.

I believe master should be in a shippable state now with the bugfix. Could you do a PyPI release when you have a chance? I believe this should be 1.1.1, and have updated the changelog.

Fixes reporting to GitHub.

Note: I'm doing this all from the web interface, have not actually run the tests myself yet, hopefully Travis will tell me if it works.

Looks like there was an edge case in how find was finding particular files. This fixes it.

Would appreciate review from @justinabrahms in case you can think of any knock-on issues this might cause.

Not quite ready for merge (only PRs are working), but wanted to see if this is something you'd like contributed.

If so, I figure you'll have some ideas about how you want to handle the extra arguments necessary for stash. Currently just passing around kwargs everywhere but obviously that's not optimal.

Since stash is always (I think) hosted privately, we need a stash_hostname. Also, stash is organized by project then by repo, so there is a project_name argument as well.

Lastly, I changed github_username/password to drop the github_ part so it can be reused.

Say I lint the following file:

requests.get('https://python.org', verify=False)

...with Bandit and Flake8. Bandit will run and return:

{"file.py": {"1": ["something about SSL"]}}

...and then Flake8 will run and return:

{"file.py": {"1": ["Module requests not found"]}}

These results will get dict.merge'd together, therefore clobbering the Bandit result with the Flake8 result, as it's a shallow merge.

This change fixes that issue by deep merging the results. Since that requires that the API is conformed to, I've updated tests so that they return valid data.

Side note – can we have a new PyPI release? I noticed that none of the recent changes are on PyPI (including the printing reporter being broken).

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.

@mghayes: Can you validate that this makes sense? Do you need more information around this to convert the JSL plugin to a separate repository? Does this feel like too much work?

It seems like this needs to be done? I'm currently getting this error when running with --no-post:

Traceback (most recent call last):
  File "/mnt/services/jenkins/workspace/imhotep/imhotep/bin/imhotep", line 9, in <module>
    load_entry_point('imhotep==1.0.1', 'console_scripts', 'imhotep')()
  File "/mnt/services/jenkins/workspace/imhotep/imhotep/imhotep/main.py", line 54, in main
    imhotep.invoke()
  File "/mnt/services/jenkins/workspace/imhotep/imhotep/imhotep/app.py", line 135, in invoke
    x, pos_map[x], violations['%s' % x])
TypeError: report_line() takes exactly 7 arguments (6 given)

Here are the signatures before and after the changes:

Let me know if I misunderstood. Thanks!

There is an error inimhotep/tools.py for version 0.4.0 installed via pip that results in TypeError:

    if not hasattr(self.response_format):
    # TypeError: hasattr expected 2 arguments, got 1

The error is here

The fix appears to be to change the source to hasattr(self, 'response_format') but the fact no-one has raised this since the code was introduced ~4 months ago makes me a bit suspicious that something odd is happening.

Python 2.6 supports directory execution (i.e. “python imhotep” instead of “python imhotep/main.py” and 2.7 supports direct package execution (“python -m imhotep” regardless of where it’s installed). In both cases, what’s needed is a file named __main__.py in the imhotep package/directory. This can make the doc simpler for both non-installed and installed usage.

So the readme says to call imhotep.py to run the script, but it looks like that file doesn't exist. At least not on master (as of c4ee47e457). So I tried instead to call imhotep/main.py, but got the following error:

Traceback (most recent call last):
  File "imhotep/main.py", line 305, in <module>
    imhotep = gen_imhotep(**params)
  File "imhotep/main.py", line 200, in gen_imhotep
    plugins = load_plugins()
  File "imhotep/main.py", line 112, in load_plugins
    klass = ep.load()
  File "/Users/streeter/.virtualenvs/tmp-2582e1239fbec948/lib/python2.7/site-packages/pkg_resources.py", line 2021, in load
    entry = __import__(self.module_name, globals(),globals(), ['__name__'])
  File "/Users/streeter/.virtualenvs/tmp-2582e1239fbec948/lib/python2.7/site-packages/imhotep_pylint/plugin.py", line 1, in <module>
    from imhotep.tools import Tool
ImportError: No module named imhotep.tools

I fixed that by first running:

python setup.py install

from within the imhotep checkout. So we should update the readme to reflect the new installation instructions.

Currently, Imhotep assumes the base branch is called master.

https://github.com/justinabrahms/imhotep/blob/046ec49f7d337fcbcd81dd30b883a3c4bcf6ecc0/imhotep/repomanagers.py#L107

This name is getting deprecated.

We should at least make this a CLI argument.

Didn't find anything like this either as an old issue or as a plugin so I decided to post it here. Perfectly happy to develop it myself as a plugin (assuming it is feasible).

I would like to have a imhotep plugin that adds a coverage difference report to PRs as a comment on top. Something like this:

Filename          Stmts    Miss    Cover     Missing
----------------  -------  ------  --------  ---------
dummy/dummy.py    -        -2      +50.00%   -2, -5
dummy/dummy2.py   +2       -       +100.00%
TOTAL             +2       -2      +50.00%

I think this will be very helpful to see test coverage improvements with each PR.

There are several tools for this including pycobertura and diffcover. Assuming pycobertura I would need to do this:

• Check master
• Check code under the given PR
• Run pytest with coverage on for master
• Run pytest with coverage on for the given PR.
• Run pycobertura diff coverage.old.xml coverage.new.xml --source1 old_source/ --source2 new_source/. This will generate a text report
• Return that from imhotep invoke(). The output would have to be for the whole PR and not for specific lines. Does imhotep supports this?

Would this be something sensible? Happy to develop it as a separate plugin but I'll need some guidance.

WARNING:imhotep.http:Error on POST to https://api.github.com/repos/xxxxxx/xxxxxx/pulls/xxx/comments. Response: {"message":"Validation Failed","errors":[{"resource":"PullRequestReviewComment","code":"invalid","field":"path"}],"documentation_url":"https://developer.github.com/v3/pulls/comments/#create-a-comment"} ERROR:imhotep.reporters.github:Error posting line to github. {u'documentation_url': u'https://developer.github.com/v3/pulls/comments/#create-a-comment', u'message': u'Validation Failed', u'errors': [{u'field': u'path', u'code': u'invalid', u'resource': u'PullRequestReviewComment'}]}

To be clear, the bot user I'm using has admin access to the organization and the upstream repo, and also for the remote third party repo. It can comment just fine, via the Github web UI.

http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule

At Thread, we adhere to the Boy Scout rule of "try and leave the world a little better than how you found it". As a result, code review comments often point to style changes in surrounding code that is not in the actual diff.

I propose to implement a Boy Scout option (name not confirmed of course, open to suggestions) that would be the number of lines either side of a diff'd line that are eligible for reporting.

This would allow the following use cases:

• Boy Scout = 0, current behaviour, only lint changes.
• Boy Scout = ~1-5, small additional linting, includes lines that show in the GitHub diffs without actually being a changed line.
• Boy Scout = ~10000, allows for incremental but aggressive, large scale style changes by considering essentially the whole file.

I plan to attempt implementing this soon, please let me know if this isn't a feature you'd accept, or if you have any thoughts about the implementation.

i've written a preliminary package: https://github.com/JerrySievert/imhotep_clangtidy

can you please give it a once-over and let me know how it looks?

thanks!

I had a problem where a lint tool plugin returned symlink paths, but imhotep was looking for the realpath.

so the plugin returned '/private/var/...', but imhotep was looking for '/var/...'

This stopped imhotep from detecting the errors here

This is mac specific.

So far I cannot tell how to determine if the folder is a symlink...the os.path.realpath does not work here :(