search

plone / plone.recipe.codeanalysis

provides static code analysis for Buildout-based Python projects, including flake8, JSHint, CSS Lint, and other code checks
https://pypi.org/project/plone.recipe.codeanalysis/
11 stars 5 forks source link

Detect explicitly installed extra checkers #149

Open reinout opened 9 years ago

reinout commented 9 years ago

Background: if you want to use jshint, you need to have it installed globally. Not every developer might have it installed (with the right version). So using buildout's node recipy and installing jshint as bin/jshint seems like a good idea. You then must tell codeanalysis where the jshint binary is with the jshint-bin option.

Idea: detect if binaries like jshint and check-manifest have been installed into buildout's bin/ folder and automatically enable them. If bin/check-manifest or bin/jshint exists, it means the buildout config explicitly installed them. Logically, it also means we want to use them.

So: if ${buildout:bin-directory}/jshint exists, automatically enable the jshint check and set jshint-bin to ${buildout:bin-directory}/jshint.

Why: less paperwork in my buildouts. Paperwork often means that it isn't getting used by my colleagues.

Risk: the first time the buildout is run, the order of parts is important. The codeanalysis part should come after the node/npm recipe and after the sysegg recipe that install the binaries. In practice, a small hint in the documentation should be enough.

Would this be something worth adding? Would it fit the project?

hvelarde commented 9 years ago

+1

saily commented 9 years ago

I was thinking about installing automatically if global executable could not be detected and links it. To keep my QA jobs fast i don't want to install it through buildout and would prefer the recipe to detect a global installed version on test-runners.

Also see: https://github.com/gawel/gp.recipe.node/issues/25 some days ago

reinout commented 9 years ago

@saily: global installs need global permissions. Buildout normally runs as a non-root user, so it cannot possibly install packages globally.

You could check if a global "jshint" or "check-manifest" is installed. But that differs from "this buildout explicitly installed jshint" and "this buildout explicitly installed check-manifest". If someone installed jshint globally, jshint would automatically be checked, even though the code might not be set up right for that kind of check.

We might want to allow that, but it doesn't fit in well with my view of buildout as being something that explicitly sets up your complete environment.

So... I'd stick with checking for "bin/jshint". And I'd not check for a globally installed jshint.

tisto commented 9 years ago

Personally I prefer the global install option. Though, I think both approaches are valid use cases. I wouldn't mind to add an option to automatically detect and pick up a locally installed jshint in "bin" as described by @reinout.

saily commented 9 years ago

@reinout i understand the permission issue very well, but i want buildout or a recipe to be intelligent enough to detect this. I think @tisto is also seeing it from the testing perspective - as i do - where you gain a lot of speed if you ship these tools globally instead of installing them in every testrun.

for example we use pre-built docker images to test in, so it's not an issue for us.

  1. global executable
  2. bin/[executable]
  3. trigger auto install and use new installed executable

I suggested to add option 3. not skipping your suggestions :-)