Multiple Projects per Repo, Eslint Enhancements, and More... #179

sjawhar · 2018-02-03T22:54:08Z

Purpose

My apologies for combining unrelated changes into a single PR. These are modifications I made for my own use, thought I'd share. I've described each change below. Feel free to merge, cherry pick, or completely discard.

Multiple Projects per Repo

Using a simple _# suffix to the name of [tool_*] sections, it's now possible to run the same linter multiple times, using different settings for each run. This can be useful when you have multiple projects in the same repo, with each client using different linting settings. For example, you might have a node.js API and React client in the same repo, and different linting settings according to the best-practices of each. You can now do this using separate [tool_eslint_#client] and [tool_eslint_#api] sections. Since most linters support a .ignore file, you can have each project ignore the files of the other, which takes care of the concerns you laid out about having to consolidate the comments before posting. I've added documentation that clarifies how to use this. Note that whatever follows after the _# is irrelevant, so long as each section has a unique name.

These changes, along with those in the next section, should address cases like #144, at least for those using eslint or PHPCS, which I updated. Minimal changes would be needed to update the other linters.

Ad-Hoc Linter Installations and Package Resolution

It really bugged me that you had to install all your linting tools into the linting bot. That means you have to keep updating the linting bot when you want to change your linting config (e.g. switch eslint configs from standard to airbnb or add a new plugin). Most languages have package managers these days, and most projects use those package managers to keep track of what the project's dependencies are. So why can't the bot just use a package.json or composer.json to get the tools it needs?

Well, now it can! Using the working_dir and install options in supported linters, the bot will run the appropriate package manager's install command (e.g. npm install for eslint or composer install for phpcs) in working-dir before linting. For supported languages, this means that you don't need to install any linting tools inside the linting bot or update it when your config changes!

This also resolves a linting issue I was having, where the bot would complain about unresolved references to imported packages (e.g. axios, sequelize, etc.).

Again, I updated eslint and phpcs, but it would be trivial to add support for the other tools that can be installed by package managers. Admittedly, this might not work for those who need access to private packages or something, but it will work for many use cases.

Minimal Docker Image

I like small Docker images. The image built using the Dockerfile in the repo is almost 1GB! So I made a minimal alpine image that only installs the packages necessary to run the linter. I also split out the linters in requirements.txt into a separate file, so that the alpine image doesn't install python linters by default.

Since I was doing this just for my own needs, I extended from the node alpine image, which others might not need. So this image should be revised to extend from the official alpine image instead. Then users can extend from the alpine version of lint-story to add only the tools they need (e.g. node, php and composer, python linters, etc.). I've published my working image under sjawhar/lint-review:alpine if you want to take a look.

Eslint Extensions

Added the ability to specify the file extensions you want to use with eslint. They were hard-coded to only .js and .jsx. Now those are just the default. Also added documentation to that effect.

Settings and Environment variables

Made more things in settings.py configurable with environment variables, reducing the chance that the user will have to write their own settings.py.

Other thoughts

On a side note, there's a misleading bit of info the current documentation:

The [tools] section is required, and linters should be a list of linters your project uses

That's not actually true. The [tools] section can be completely omitted if you use [tool_*] sections. You can see it in the code below. The linters in the tools section are used to populate a dictionary but are immediately overwritten by the [tool_*] sections:

lint-review/lintreview/config.py

Lines 172 to 187 in 131f3cf

    
           linters = [] 
        
           if parser.has_section('tools'): 
        
               linters = comma_value(parser.get('tools', 'linters')) 
        
           if parser.has_section('fixers'): 
        
               data['fixers'] = dict(parser.items('fixers')) 
        
           # Setup empty config sections 
        
           for linter in linters: 
        
               data['linters'][linter] = {} 
        
           for section in parser.sections(): 
        
               if not section.startswith('tool_'): 
        
                   continue 
        
               # Strip off tool_ 
        
               linter = section[5:] 
        
               data['linters'][linter] = dict(parser.items(section)) 
        
           self.update(data)

…running eslint

…or same tool

stickler-ci · 2018-02-03T22:54:17Z

lintreview/tools/eslint.py

@@ -17,14 +18,16 @@ class Eslint(Tool):
    def check_dependencies(self):
        """See if ESLint is on the system path.
        """
-        return in_path('eslint') or npm_exists('eslint')
+        return in_path('eslint') or npm_exists('eslint', cwd=self.get_working_dir())


E501 line too long (84 > 79 characters)

stickler-ci · 2018-02-03T22:54:17Z

lintreview/tools/eslint.py

-        if npm_exists('eslint'):
-            cmd = os.path.join(os.getcwd(), 'node_modules', '.bin', 'eslint')
+        if npm_exists('eslint', cwd=self.get_working_dir()):
+            cmd = os.path.join(self.get_working_dir(), 'node_modules', '.bin', 'eslint')


E501 line too long (88 > 79 characters)

stickler-ci · 2018-02-03T22:54:17Z

lintreview/tools/phpcs.py

@@ -22,7 +22,7 @@ def check_dependencies(self):
        """
        See if PHPCS is on the system path.
        """
-        return in_path('phpcs') or composer_exists('phpcs')
+        return in_path('phpcs') or composer_exists('phpcs', cwd=self.get_working_dir())


E501 line too long (87 > 79 characters)

stickler-ci · 2018-02-03T22:54:17Z

settings.sample.py

-GITHUB_AUTHOR_NAME = 'lintreview'
-GITHUB_AUTHOR_EMAIL = '[email protected]'
+GITHUB_AUTHOR_NAME = env('LINTREVIEW_GITHUB_AUTHOR_NAME', 'lintreview')
+GITHUB_AUTHOR_EMAIL = env('LINTREVIEW_GITHUB_AUTHOR_EMAIL', '[email protected]')


E501 line too long (85 > 79 characters)

markstory

It would be good to have updated tests for the changes to the linter tools.

markstory · 2018-02-03T23:03:24Z

lintreview/tools/eslint.py

+
+        if self.options.get('install'):
+            output = run_command(
+                ['npm', 'install'],


My concern with installing application dependencies is that many react/ember/angular applications have build dependencies that are in the 100-400MB range. Downloading all those dependencies on each review will add a significant runtime cost and potentially create bandwidth usage problems.

It's optional, so the user can decide as they see fit.

Also, with node 8 and package-lock.json, installs are much quicker than they were in the past.

Also, installing the project's dependencies was how I resolved the issue of the linter generating erroneous "unresolved reference to PACKAGE" errors. I suppose one could disable that linting rule, but then you'd miss real cases of unresolved references.

One way to deal with the extra bandwidth might be to put in more logic around cleanup. Instead of deleting the repo after every lint, maybe it's deleted when the PR is closed or merged? Maybe there's a configurable cleanup frequency (e.g. every day, week, etc.)?

I think the npm cache actually takes care of the issue of bandwidth, to a large degree

markstory · 2018-02-03T23:06:35Z

lintreview/tools/phpcs.py

+            output = run_command(
+                ['composer', 'install'],
+                ignore_error=True,
+                cwd=self.get_working_dir())


I have similar concerns with PHP dependencies as I have with running npm install around time and cost.

Do you often find that your PHP applications require additional composer packages to install their PHPCS rules?

As with npm, it's optional.

As for additional composer packages, I'm not really familiar with PHPCS. Just wanted to demonstrate on a non-node linter. The point is that a team could add a PHP project to their existing projects without having to touch the linting bot.

I have plans in the near future to extract the various tool runtime environments into small standalone docker images removing the need for a kitchen sink image entirely. I feel this change would not be necessary if phpcs was a separate image people could pull down as needed.

I would prefer to not add features that will be removed in the short term.

Are you saying the linting bot would start docker containers for linting environments? Or that you would have different "flavors" of the linting bot docker image, each with different tools?

That the review jobs would spawn containers to run each tool on the repository being reviewed.

Awesome! That's actually how I initially thought I was going to solve my problem when I started this work, but when I saw how easy it was to get the functionality I needed by just re-jiggering a few lines...

Spawning containers might solve some problems, but wouldn't you still need to have all your eslint config and plugin packages for all your projects pre-installed in the linter docker image? And update the image when your config changes? I also learned that this does not only apply to eslint. With PHPCS, for example, you can add new "sniffs" as composer packages.

you still need to have all your eslint config and plugin packages for all your projects pre-installed in the linter docker image?

Yes you would. It doesn't help address the 'unresolved imports' for eslint. But it would allow lintreview users to build their own docker images that contain the custom sniffs/packages they use.

I think that would be great for a lot of people. However, I also think many would much rather not have to maintain separate docker images. I'm already doing the work of keeping track of needed dependencies when I update my package.json/composer.json/etc.. I wouldn't want to then have to go and update another tool with those same changes.

I hear you on the pain of setting up docker images. For the node based projects I think the install option is one of the only ways to get around the unresolved imports due to how babel & eslint handle those checks. However, with PHP and other toolchains linting is far less invasive and changes far less frequently than in Javascript projects.

markstory · 2018-02-03T23:07:10Z

lintreview/utils.py

@@ -19,26 +19,24 @@ def in_path(name):
    return False


-def npm_exists(name):
+def npm_exists(name, cwd=os.getcwd()):


This default value will capture the cwd() when the module is imported. I don't think that's what you want.

I must not fully understand how it works, then. Could you explain more? I could use None as default and set to os.getcwd() if None is passed.

The current default value will be created when this module is imported, not each time the function is run. Using None and getting cwd if none is passed will work as you want.

I see, thank you.

sjawhar · 2018-02-03T23:27:59Z

Agreed that updated tests would be good. I hesitated in opening this PR because I knew I wouldn't have time to add them for a while. I'll try, when I get the chance.

codecov-io · 2018-02-06T05:32:21Z

Codecov Report

Merging #179 into master will decrease coverage by 0.31%.
The diff coverage is 69.04%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
- Coverage   85.97%   85.65%   -0.32%     
==========================================
  Files          40       40              
  Lines        2167     2196      +29     
==========================================
+ Hits         1863     1881      +18     
- Misses        304      315      +11

Impacted Files	Coverage Δ
lintreview/tasks.py	`29.54% <100%> (ø)`	⬆️
lintreview/utils.py	`85.71% <100%> (+0.86%)`	⬆️
lintreview/tools/phpcs.py	`66.21% <11.11%> (-5.85%)`	⬇️
lintreview/web.py	`83.01% <50%> (-1.3%)`	⬇️
lintreview/tools/__init__.py	`88.19% <75%> (-0.78%)`	⬇️
lintreview/tools/eslint.py	`97.26% <86.66%> (-2.74%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b6d29a...94edb43. Read the comment docs.

…int and phpcs

stickler-ci · 2018-02-06T21:50:10Z

lintreview/tools/eslint.py

@@ -51,7 +66,8 @@ def process_fixer(self, files):
        output = run_command(
            command,
            ignore_error=True,
-            include_errors=False)
+            include_errors=False,
+            cwd=working_dir)


F821 undefined name 'working_dir'

…ectoryies

Pull in a part of #179 that allows eslint to configure which extensions of files are checked.

Refs #179

Pull in some changes from #179

markstory · 2018-09-03T15:24:24Z

Closing as I pulled in the desired pieces of this. Thank you!

Sami Jawhar and others added 12 commits February 2, 2018 11:13

Added ability to change working directory and run npm install before …

9342521

…running eslint

Updated readme with eslint working_dir and install config options

a8f334c

Strips everything following _# from tool, allowing multiple ocnfigs f…

12c6aab

…or same tool

Added documentation of multiple _# sections

fabf437

Added phpcs working_dir and install

5bd86dc

Simplified handling of working_dir

760c9c9

Broke linters into separate requirements.txt

d944616

Added alpine dockerfile

4104343

Added openssl and ran pip install after copy

27ab248

Fixed install output typo

8716d6e

Added support for extensions in eslint

cc7ece1

Added extensions to eslint readme and fixed phpcs

d17a8fa

stickler-ci reviewed Feb 3, 2018

View reviewed changes

sjawhar added 4 commits February 3, 2018 15:00

Update README.mdown

f7553ae

Fixed line length in PHPCS

8ba8e31

Fixed line length in eslint

16ba05f

Shortened name of github comment author envs

8856c87

markstory requested changes Feb 3, 2018

View reviewed changes

markstory self-assigned this Feb 3, 2018

sjawhar added 2 commits February 5, 2018 08:48

Changed cwd default to None in npm_exists and composer_exists

7b8494d

Added install of linter packages to travis build

34f95c5

sjawhar and others added 6 commits February 6, 2018 12:22

Rebase alpine image on node

6a15d48

Updated alpine package versions

5a10202

Fixed typo in gcc version in alpine image

ed6c243

Added curl to alpine docker image

31dc615

Updated healthcheck script to work with sh

7b28c6e

Made sure linting and fixing commands are run in working_dir, for esl…

9c2a8b3

…int and phpcs

stickler-ci reviewed Feb 6, 2018

View reviewed changes

Sami Jawhar and others added 11 commits February 6, 2018 21:55

Fixed typos in working dir in eslint tool

46dd832

Check that line is digit before converting to int

71142a2

Merge branch 'master' of github.com:markstory/lint-review

f029eea

Added debug for working dir

55cb6b0

Added debug output of linting command

2c5be2e

updated alpine packages

9c7b1ad

Added documentation about multiple projects, ignoring, and nested dir…

3e64e76

…ectoryies

Handled case where lintrc does not exist or is empty

6a7b068

Updated gunicorn to fix stdout/stderr logging

b2351c1

Merge branch 'master' of github.com:markstory/lint-review

57e85d9

Upgraded to celery 4.1.0

94edb43

markstory added a commit that referenced this pull request Mar 7, 2018

Add configuration option for extensions on eslint.

e601909

Pull in a part of #179 that allows eslint to configure which extensions of files are checked.

markstory added a commit that referenced this pull request Mar 7, 2018

Add environment variables for author name/email

4ee1881

Refs #179

markstory added a commit that referenced this pull request Mar 8, 2018

Merge pull request #189 from markstory/eslint-extensions

e4a827c

Pull in some changes from #179

markstory closed this Sep 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple Projects per Repo, Eslint Enhancements, and More... #179

Multiple Projects per Repo, Eslint Enhancements, and More... #179

sjawhar commented Feb 3, 2018 •

edited

Loading

stickler-ci Feb 3, 2018

stickler-ci Feb 3, 2018

stickler-ci Feb 3, 2018

stickler-ci Feb 3, 2018

markstory left a comment

markstory Feb 3, 2018

sjawhar Feb 3, 2018

sjawhar Feb 3, 2018

sjawhar Feb 3, 2018

sjawhar Feb 4, 2018

markstory Feb 3, 2018

markstory Feb 3, 2018

sjawhar Feb 3, 2018

markstory Feb 4, 2018

sjawhar Feb 4, 2018

markstory Feb 5, 2018

sjawhar Feb 5, 2018

markstory Feb 5, 2018

sjawhar Feb 5, 2018

markstory Feb 6, 2018

markstory Feb 3, 2018

sjawhar Feb 3, 2018

markstory Feb 4, 2018

sjawhar Feb 4, 2018

sjawhar commented Feb 3, 2018

codecov-io commented Feb 6, 2018 •

edited

Loading

stickler-ci Feb 6, 2018

markstory commented Sep 3, 2018

	linters = []
	if parser.has_section('tools'):
	linters = comma_value(parser.get('tools', 'linters'))

	if parser.has_section('fixers'):
	data['fixers'] = dict(parser.items('fixers'))
	# Setup empty config sections
	for linter in linters:
	data['linters'][linter] = {}
	for section in parser.sections():
	if not section.startswith('tool_'):
	continue
	# Strip off tool_
	linter = section[5:]
	data['linters'][linter] = dict(parser.items(section))
	self.update(data)

Multiple Projects per Repo, Eslint Enhancements, and More... #179

Multiple Projects per Repo, Eslint Enhancements, and More... #179

Conversation

sjawhar commented Feb 3, 2018 • edited Loading

Purpose