Coder Social home page Coder Social logo

lint-trap's Introduction

lint-trap

This module contains standardized linting rules to be used across all projects at Uber that contain JavaScript.

Usage

npm install --save-dev lint-trap
./node_modules/.bin/lint-trap <list of file or folder paths>

It is recommended that you add lint-trap to the scripts property of your project's package.json manifest file, like so:

"scripts": {
    "lint": "lint-trap",
}

... and then you can invoke it by executing npm run lint.

Tips and Tricks for Legacy Projects

Using lint-trap to get a legacy project with many thousands of lines of code in line with the standards enforced by lint-trap can be a daunting task. To make the process as painless as possible, this guide will give you practical advice on tackling all your lint errors and warnings in bite-size chunks.

Linting Support in Text Editors

lint-trap is very new so support for text editor plugin is still immature. Plugin support already exists for SublimeText 3 and Syntastic (vim). Plugin support for flycheck in emacs is planned.

For more information and workarounds if your editor is not yet supported, see the documentation on code editor support.

Attenuating Rules

Since lint-trap is meant to enforce good coding style and consistency across many projects from the same organization, you cannot turn rules off completely. However, when using lint-trap in legacy projects without any linter or with different linting rules, it is useful to be able to downgrade the warning severity from error to warning so you can pay down linting technical debt over several commits. Because of this, lint-trap supports adding a .lintrc file to your project.

For more information on configuring a .lintrc file or command-line options, see the configuration docs.

If you're using lint-trap in a legacy project, you should also check out the legacy project tips.

Indentation

lint-trap dynamically detects whether it should enforce a 2-space or 4-space softtab indent rule. It does this by inspecting a reference file to detect the indentation used in that reference file, and then enforces the detected indentation on all the files it is linting.

For more information, see the documentation on indentation

Warnings vs. Errors

Almost all lint-trap rules produce errors. If you see an error, you should fix it. However, a few rules, notably max-statements and complexity produce warnings. The reason these rules produce warnings is because their goal is to highlight code where there might be too much going on within a single block of code.

Long, complex code blocks are likely to be less readable and therefore less maintainable. When you see a warning, you should investigate the code in question and deliberately determine if the code should be refactored or if the code is okay as is. There exist cases where lint-trap will return a warning, but where the code is acceptable as is. For example, you might find that in some tests you have many assertions and you go beyond the maximum number of statements allowed. In this situation you might determine that many assertions are not only acceptable but necessary without reducing readability and comprehension. If you encounter code that should be considered acceptable, you can add an inline linter directive to tell lint-trap to ignore that code for that particular warning.

Since all the lint rules that produce warnings are currently handled by eslint all you need to do is add the following two lines around your code, where lint-rule-id is the string eslint uses to identify a particular rule such as max-statements:

/*eslint-disable lint-rule-id*/
/* the code producing the warning here */
/*eslint-enable lint-rule-id*/

Contributing

Contributions to lint-trap are welcome, but since lint-trap is effectively a module that encapsulates a set of opinions and throws errors and warnings at you when you violate those opinions, there is a lot of room to debate over what color to paint our bikeshed.

Before you begin filing an issue to argue why you think your color of paint is superior, it's worth knowing how the current set of rules were determined. Javascript enginerrs from several teams with different needs (both front-end engineers and back-end NodeJS engineers) were the first to go through all the rules, try them out and debate the merit of each. This group is consists of developers that collectively have seen tons of code and tons of bugs in production systems at scale that arose from poor choice of coding style and conventions.

The rules and the reasoning behind each should all be documented or will be over time. Before we bikeshed over a rule, please check the rules documentation. If a rule hasn't been documented or hasn't yet been documented adequately, open an issue asking for clarification and better documentation first. If a rule has been documented and you still disagree, there is one task you must perform before you are allowed to bikeshed. You must first read Clay Shirky's essay A Group is its Own Worst Enemy. At the end of the day, we all love bikeshedding, but we would like to keep it to a minimum, so we can all get work done.

Why lint-trap

If there are all these other linters out there like JSHint, ESLint and jscs, why does lint-trap exist?

Uber like many large companies using JavaScript has a LOT of projects all with their own set of .rc files (pronounced dot·arc) for each linter. These configuration files are almost always copypasta-ed from a a previous project and they all evolve and mutate over time as each developer gets their hands on a project and make their own changes. This means that over time, every project ends up with its own style adding unnecessary friction to collaboration and working across many projects.

lint-trap aims to be a zero-configuration linter as much as possible, and requires no configuration in brand new projects. The few configuration options it offers exist solely to help legacy projects reach 100% linting conformance piecemeal.

For more information on why we think lint-trap is valuable see the documentation describing the philosopy behind lint-trap

Using lint-trap in non-Uber projects

lint-trap currently includes hard-coded lint rules in use at Uber. If you work at Uber and work with JavaScript, you should have lint-trap in your project. If you don't work at Uber, you're welcome to use lint-trap in your project as is.

If you want lint-trap to support a different rules for your organization, we first ask that you try out the rules with which lint-trap already comes. The rules have been discussed at length by many engineers and are well thought out for any environment where lots of devs need to work together and need to feel at home in each others' code. Spend some time with the existing rules and reading the explanation for their existance in the docs and we're pretty sure you'll find them to be a pretty good balance for large engineering organizations.

If, after trying out our lint-rules for some time, you feel that you still need to support different rules, please comment on this issue. We will be glad to work with you to add support for this lint-trap.

License

The MIT License (MIT)

Copyright (c) 2014 Uber Technologies, Inc.

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

lint-trap's People

Contributors

andrewdeandrade avatar dawsbot avatar jcorbin avatar malandrew avatar mishabosin avatar raynos avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Forkers

raynos

lint-trap's Issues

jshint missing new prefix

lint-trap index.js 
home/raynos/projects/mercury/index.js
    error  jshint    50:16   Missing 'new' prefix when invoking a constructor.  W064
    error  jshint    53:12   Missing 'new' prefix when invoking a constructor.  W064

jshint should be configured to not complain about this.

I believe we need to default newcap: false

Project conversion helpers

Implement some helpers to convert a project to lint-trap that currently uses jshint, eslint and jscs configuration and ignore files:

Subtasks:

  • Find and consolidate all .ignore files as .lintignore
    • jscs is a special case (excludeFiles in .jscsrc)
  • purge linter configuration and ignore files from repo
  • purge linter scripts from package.json
  • purge jshintConfig property from package.json if exists

no-underscore-dangle: 2

This rule should not be on.

We have a ton of code using this pattern, including node core itself.

.lintrc file that allows downgrading errors to warnings

The purpose of lint-trap is to allow many linting rules from various linters to be encapsulated into one ruleset so you can use all these rules across many projects without resorting to copypasta.

Because of this, it does not make sense to allow rules to be turned on/off, since all rules should always be on.

However, an organization or individual might have many legacy projects that don't conform to all the linting rules encapsulated by lint-trap. In this situation, a developer may prefer to downgrade a linting rule from an error (exit code 1) to a warning (exit code 0) so that tests will pass. This makes it easier for developers to include lint-trap in their project without being "continuous integration" blocked from delivering time-sensitive features as they get their linting issues addressed piecemeal over several commits.

related:

  • if no warnings are generated for rule set to warning, the linter should through an error that a rule is unnecessarily set to warning

.lintwarn file?

Allow downmodding to warn using .ignore file semantics.

When trying to add lint-trap to an existing project, it became clear that a .lintrc and .lintignore file file is probably not the best approach for getting lint-trap into legacy projects. Ignoring files that you want to lint slowly over time leads to an out-of-sight and out of mind situation. Downmodding legacy files to warning prevents this out-of-sight-out-of-mind but still allows developers to keep committing code as they clean things up over time.

Ideally lint warnings are only downmodded for files that committed but unmodified (i.e. git diff produces no output)

smarter way to resolve indentation of current project

The current approach of resolving the indentation of the current project based on package.json:main is naive in that it does allow the linter to be run on individual files or to lint from content piped in on stdin.

Use a smarter way to determine the indentation of a project, possibly using .editorconfig settings if available or allowing the indentation to be set in the .lintrc file. Fallback to detecting indentation if the indentation is not set explicitly via either method.

Do not use the indentation of the current file as reference since it is entirely possible to lint a single file using one indent style while the rest of the project uses something else. In the case that indentation cannot be determined any way, either the indentation of the current/first file should be used or the indentation average across all the files being linted should be used (this will be slow, but a last resort. correctness is preferable to speed since this is an edgecase).

line-length in lintrc

The current --line-length CLI flag approach is not workable for linter plugins like SublimeLinter. This needs to be setable in the lintrc file. (ref: T36892)

dev-mode or watch-mode

It would be useful to have a dev-mode or watch mode where errors downgraded to warnings so your test suite runs when practicing TDD.....

When this feature is available, the no-console rule should be upgraded back up to an error.

This issue, #64 , is related because both might be implementable the same way: via an only flag which can either be warning or error:

--only=error or --only=warning

Question: Is process.env.NODE_ENV an appropriate way to set everything to warning?

An (eslint?) rule forbidding exposing values outside of their original scope. (scope sandboxing?)

This is similar to global detection, and would really help with accidental critical session variable leaks, for example.

...
var someData = { }; 
function someHandler(req, res, next) {
  // Never bring anything in req outside its scope. 
  someData.state = someData.state || req.someSensitiveData;

  // Now we risk exposing some sensitive data to a
  // request session to which it did not originally belong!
  res.send(someData);
}
...
$ lint-trap
server/index.js
    error  jscs       1:1    Illegal scope                               disallowMoveScopes

I'm still figuring out what's the idiomatic way to call this anti-pattern.

cc @malandrew @Raynos

jshint validthis: true

Because we have strict mode we should tell jshint to go away with its this violation warning.

Let's enable validthis: true since that gives warnings about code thats not broken.

lint-trap + sublime linter

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: Unexpected "e" at position 0 in state START
    at Parser.proto.charError (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/jsonstream2/node_modules/jsonparse/jsonparse.js:89:16)
    at Parser.proto.write (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/jsonstream2/node_modules/jsonparse/jsonparse.js:118:23)
    at DestroyableTransform._transform (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/jsonstream2/src/jsonstream2.js:34:12)
    at DestroyableTransform.Transform._read (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:184:10)
    at DestroyableTransform.Transform._write (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:172:12)
    at doWrite (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:237:10)
    at writeOrBuffer (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:227:5)
    at DestroyableTransform.Writable.write (/home/raynos/projects/nvm/v0.10.26/lib/node_modules/lint-trap/node_modules/through2/node_modules/readable-stream/lib/_stream_writable.js:194:11)
    at write (_stream_readable.js:583:24)
    at flow (_stream_readable.js:592:7) 

When running lint-trap from sublime sometimes this happens. not really sure what that means

message.type => message.severity

type is an ambigious property name. refactor to use the property name severity.

Possible severity levels are error, warning and info.

Case statement should not require curly braces

Getting an error for missing curly braces when case statements never require curly braces
http://www.w3schools.com/js/js_switch.asp

   Error  (requireCurlyBraces) jscs.requireCurlyBraces
    Case statement without curly braces

              37 
              38     switch (action.actionType) {
              39       // server actions
    >>>       40       case AppConstants.VEHICLE_FROM_CLASS_VALIDATION:
              41         _vehicleClassInfo.fromClass = data;
              42         VehiclesStore.emitChange();
              43         break;

indent rule not finding first file when not main

when there is no package.json:main, lint-trap is not falling back to the first js file found.

furthermore, there needs to be a better error when no js file is found (or it should exit silently?)

error code through stream

create a through stream that determines the error code to return. also count the number of errors and warnings.

use this to refactor the reporters into streams with a standard pipeable stream interface instead of bulk writers or writers that operate on each data event.

Eliminate duplicate rules

Since lint-trap supports 3 linters, there are some duplicate rules. When the same error is thrown by more than one linter, prefer eslint over jscs and jshint and prefer jscs over jshint.

Known duplicates:

eslint.eol-last > jscs.requireLineFeedAtFileEnd
eslint.space-before-blocks > jscs.requireSpacesInFunctionDeclaration
eslint.no-extra-semi > jshint.W032
eslint.no-undef > jshint.W117
eslint.no-trailing-spaces > jscs.disallowTrailingWhitespace
eslint.semi > jshint.W033
eslint.no-multiple-empty-lines > jscs.disallowMultipleLineBreaks
eslint.max-len > jscs.maximumLineLength

If you find duplicate rules, please mention them in comments below.

Pre-commit hook that requires 100% linting on touched files.

Issue #20 allows engineers to make some rules only give lint warnings instead of lint errors that break the build. That feature is being added to give developers the flexibility to bring legacy projects up to spec over time instead of all at once. However, that feature is not meant to allow people a convenient way to indefinitely ignore linting rules.

One feature that mitigates this problem is to only allow errors to be reduced to warnings for files that are untouched in the currently staged commit. This would help make sure developers take the time to get code they've just touched and are presently familiar with to satisfy the linting rules.

Option to turn all warnings into errors

Having warnings printed as part of a pre-commit npm test BUT not fail the precommit is bullshit.

In fact warnings are bullshit, it either errors or I dont see it.

links to error docs per rule

Since our documentation often contains explanations for errors, workarounds and suggestions, it would be nice if, when an error is encountered, that the linter spits out a link to the docs for that rule

compact reporter

Compact reporter like the one that eslint has. This is required for text editor linter plugins like SublimeLinter3

eslint `func-names` rule and jscs

Currently we use a mixture of jscs whitespace before/after the function keyword and an opening parenthesis rules to force developers to name functions. Unfortunately, this leads to cryptic lint errors where the developer fixes the lint rule only to get the opposing lint error. Instead of this, the eslint func-names rule should be set to error level (2) and the jscs rules should be modified so that only those that are still relevant after a function has been named are still relevant.

CLI: usage, help and version

Currently the command line does not yet do anything with the following flags: -v, --version, -h and --help, nor does it print usage information when used incorrectly.

eslint: "curly" rule -> warning

The jshint rule curly is currently disabled because it was considered to expensive to refactor in the past. It should be left disabled in jshint, but it should produce a warning in eslint. This will allow tests to pass, but spit out warnings at the developer prompting him to fix them.

disable jshint: "defined but never used error"

Currently, there is duplicate linting going on between eslint and jshint with respect to defined, but unused tokens. The .jshintrc option in question is unused.

Since there is no need to give users two warnings, disable the rule in jshint in favor of letting the eslint rule catch the error.

Support rules from different Organizations

Lint-trap currently ships with the Uber linting rules hard-coded. Provide a way for other organizations to add a "lint-trap" property to their package.json file where they can provide the URL to a git repo or tarball containing the lint .rc files used by their organization. Lint-trap should install and use these rules if this property is present.

.lintrc jshint newcap

I tried to configure jshint newcap to false in .lintrc but it did not work as expected.

The newcap jshint error is still an error.

Support sublime-linter and syntastic

Automate supporting SublimeLinter (sublime Text) and Syntastic (vim) code editor plugins.

Version 1: Implement a post-install script that automatically sets up a parent project.

  • check if lint-trap has been installed in a project by checking if parent
    folder is node_modules/, then checking if parent of node_modules/
    contains lint-trap in the devDependencies of the package.json file.
    • If lint-trap has been installed:
      • add .jshintrc, .eslintrc and .jscsrc to .gitignore
      • copy .jshintrc, .eslintrc and .jscsrc from ./node_modules/lint-trap/lib/rc/ to the root folder so the plugins can find the appropriate configuration file.

Version 2: Create SublimeLinter and syntastic plugins for lint-trap

Line length project specific?

Should line length of 80 vs 120 be project specific?

Personally, I think 80 really should be adequate, especially if all other lint-trap rules are being followed. Many rules help keep your code flush left (callback nesting limits, func-decs preferred over func-exprs, maximum nesting, etc.).

cc: @Raynos @lxe @mlmorg @addisonedwardlee @ross- @marthakelly @Matt-Esch @sh1mmer

If we do allow flexibility on line lengths, how can we determine line length of a project reliably without having to hard code it in a configuration file?

lint-trap needs a timeout.

Sometimes lint-trap just times out and hangs forever. This is hard to reproduce. This causes the text editor to also hang.

lint-trap needs to timeout.

JSON Reporter

Create a JSON reporter that can be piped to another unix command.

lintrc overrides entire object instead of subproperties

I want to override a subproperty in jscs, such as requireCurlyBraces specific to switch and case. That is currently not possible.

Doing this

{
    "jscs": {
      "requireCurlyBraces": false
    }
}

will remove the requirement from everything, and there's no way to specify a specific subproperty to remove.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.