Skip to content

Commit dcaeb6d

Browse files
skye2k2skye2k2
authored
Update linting dependencies & documentation (#7)
Update all linting subdependencies. Add new rules, trigger, and enforce via tests. Set defaults for some rules. Add best practices and examples for managing linting in varying projects. Release version 5.0.0.
1 parent 7c992e6 commit dcaeb6d

File tree

8 files changed

+261
-83
lines changed

8 files changed

+261
-83
lines changed

‎CODEOWNERS

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#########################################################################################
2-
# Code Ownership for fs-demo
3-
# All files owned by tw-gold
2+
# Code Ownership for eslint-config-tree
3+
# All files owned by tw-gold (but really Clif)
44
#
55
# See https://help.github.com/en/articles/about-code-owners for information on how to use
66
#########################################################################################

‎README.md

+91-4
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
This is a shared configuration for all Tree repositories. Contains overrides and enhancements on top of the base configuration located at [https://github.com/fs-webdev/eslint-config-frontier](https://github.com/fs-webdev/eslint-config-frontier).
44

5-
Utilizes the following plugins:
5+
Why? Because we believe in linting, and we have become converted to the additional rules enforced by the following plugins:
66

77
- [eslint-plugin-bestpractices](https://github.com/skye2k2/eslint-plugin-bestpractices)
88
- [eslint-plugin-deprecate](https://github.com/AlexMost/eslint-plugin-deprecate)
@@ -55,10 +55,84 @@ Utilizes the following plugins:
5555

5656
1. Add both `tree` and the frontier eslint configuration of your choice as Code Climate `prepare` resources (see: [extended eslint docs](https://www.familysearch.org/frontier/legacy/ui-components/eslint-config-frontier/)).
5757

58-
1. Set this simplified eslint configuration as the chosen config in your Code Climate's `plugins`: `eslint`: `config`: `config`.
58+
1. Set this simplified eslint configuration as the chosen config in your Code Climate's `plugins`.
59+
<pre><code>plugins:
60+
eslint:
61+
enabled: true
62+
channel: "eslint-6"
63+
config:
64+
config: .codeclimate.eslintrc.js
65+
extensions:
66+
- .html
67+
- .js
68+
- .json
69+
ignore_warnings: true
70+
</code></pre>
5971

6072
1. Enjoy.
6173

74+
## HOWTOs:
75+
76+
### How to override linting rules for a directory and all of its contents:
77+
78+
Add an `eslintrc.js` file to that directory with the necessary overrides, like so:
79+
80+
```
81+
module.exports = {
82+
rules: {
83+
'bestpractices/no-eslint-disable': 'off|warn|error',
84+
}
85+
}
86+
```
87+
88+
### How to override linting rules for specific files:
89+
90+
Add an `overrides` section to your `eslintrc.js` file to target those files with the necessary overrides, like so:
91+
92+
```
93+
overrides: [
94+
{
95+
files: ['*.stories.js', '*.test.js'],
96+
rules: {
97+
// We do not need to enforce selector rules in test/demo files
98+
'test-selectors/button': 'off',
99+
'test-selectors/onChange': 'off',
100+
},
101+
},
102+
],
103+
```
104+
105+
### How to disable a linting rule inline without triggering the `no-eslint-disable` rule:
106+
107+
Utilize a file linting config modifier like so:
108+
109+
```
110+
/* eslint no-console: "off" -- node scripts use the console */
111+
112+
```
113+
114+
Note that `--` comments are permitted and a good idea to include.
115+
116+
<!--
117+
DOES NOT CURRENTLY WORK, AND bestpractices/no-eslint-disable SHOULD PROBABLY BE MODIFIED TO TAKE THIS INTO ACCOUNT.
118+
Or disable BOTH the desired rule and the no-eslint-disable rule:
119+
120+
```
121+
// eslint-disable-next-line bestpractices/no-eslint-disable, no-console
122+
```
123+
-->
124+
125+
### How to deal with `Definition for rule '{RULE}' was not found.` errors:
126+
127+
This is a known state when submitting a new file to Code Climate for the first time, since they do not support all of the linting extensions we wish to use. If you are seeing these warnings when linting locally, you likely have `eslint` installed globally, but not the additional dependency. We do not recommend running `eslint` globally for this reason (see: https://github.com/eslint/eslint/issues/6732). All Tree repositories should include all dependencies required to be able to run `eslint` locally in their respective directories.
128+
129+
### How to do even trickier things with linting configuration:
130+
131+
Just read the manual: https://eslint.org/docs/7.0.0/user-guide/configuring
132+
133+
<details>
134+
<summary>Maintenance Notes</summary>
135+
62136
## Testing/Updating:
63137

64138
Occasionally, there may be an update which breaks a rule in particular or linting in general. To this end, when running `npm test`, we output the current linting results to a text file, clean it up a little, and employ ava to run a snapshot comparison unit test to determine if our linting output has changed from the previous run.
@@ -67,14 +141,27 @@ If there has been a change (say you added a new rule, or there is a new valid vi
67141

68142
## Notes
69143

144+
- As noted in the `Testing/Updating` section, the only validation we do is to run linting against a file with a set of known failures. So we make sure to run `npm test` via a pre-push hook, and releases are automatically performed by a GitHub webhook.
70145
- Because this is a public repository, there are complications in adding references to private services and communications channels, so there is no Travis CI build and no Code Climate integration.
71-
- Coverage reporting ends up reporting on `lint-output.js`, instead of `index.js`, and so is also not used.
72-
- As noted in the `Testing/Updating` section, the only validation we do is to run linting against a file with a set of known failures. So we make sure to run `npm test` via a pre-push hook.
146+
- Coverage reporting ends up reporting on `lint-output.js`, instead of `index.js`, which is unhelpful, and so is also not used, for now.
147+
148+
</details>
73149

74150
## Changelog:
75151

152+
<details>
153+
<summary>Version 5 </summary>
154+
155+
- Update all linting subdependencies.
156+
- Add new rules.
157+
- Set more reasonable defaults for some rules.
158+
- Add best practices and examples for managing linting in varying projects.
159+
160+
</details>
161+
76162
<details>
77163
<summary>Version 4 </summary>
164+
78165
- `eslint-plugin-no-only-tests` & `eslint-plugin-no-skip-tests` are redundant to to newly-implemented `jest/no-focused-tests` & `jest/no-disabled-tests` and have been removed.
79166

80167
</details>

‎demo/.eslintrc.js

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// Example directory override (still has access to all configuration, extensions, and plugins at the root level, but overrides the settings for this directory and below, if no other .eslintrc.js file exists)
2+
// Common uses are to loosen our normally tight linting ruleset for code that is under development so that TODOs and the like do not crowd out more important warnings (provided that such overrides are removed upon production release)
3+
module.exports = {
4+
rules: {
5+
'bestpractices/no-eslint-disable': 'error',
6+
'sonarjs/no-duplicated-branches': 'error'
7+
}
8+
};

‎demo/example.js

+79-19
Original file line numberDiff line numberDiff line change
@@ -1,53 +1,113 @@
11
/* Example of a broken JS file that should trigger the additional rules contained in ./index.js */
22

3+
/* eslint no-console: "off" -- node scripts use the console, so disable for the whole file */
4+
35
/*
4-
* Since developers have the ability to disable linting in-line, we keep track of the times where this is done.
5-
*/
6+
* Since developers have the ability to disable linting in-line, we keep track of the times where this is done, because if done irresponsibly, this is a significant code smell.
7+
*/
68
// eslint-disable-next
79

810
// fixMe: Actually make this work
911
// todo: Add documentation
1012
// Hack: Note that these work, regardless of case
1113
// Here be Dragons
1214

13-
/* Tree considers the following as errors:
14-
no-unused-vars
15-
*/
16-
17-
/* Tree considers the following as warnings:
18-
no-undefined
19-
*/
20-
21-
function functionWithoutJSDocWarningsBecauseTheSectionWasCompletelyExcluded (params) {
15+
function functionWithoutJSDocWarningsBecauseTheSectionWasCompletelyExcluded () {
16+
console.log('ASHLDKFJHASKFJSDHFKJSDHFKLSDJHFLJKSDHFLKSDJFHKSDLJFHLSDKJF');
17+
return true;
2218
}
2319

2420
/**
2521
* @description a somewhat valid description
2622
* @param {hobject} a
23+
* @param params
2724
* @param b
2825
* @returns
2926
*/
3027
function ONE_FUNCTION_TO_BRING_THEM_ALL_AND_IN_THE_DARKNESS_BIND_THEM (params) {
3128
}
3229

33-
let myPromise = new Promise();
30+
const myPromise = new Promise();
3431

3532
myPromise.then((a) => {
36-
if (a) {
37-
return Promise.resolve()
33+
if (true === false) {
34+
return Promise.resolve();
3835
} else {
39-
forgotToReturn()
36+
forgotToDefine();
4037
}
41-
})
38+
});
4239

43-
let variable = (true) ? true : true;
40+
const variable = (true) ? true : true;
4441

45-
if (window === undefined && window === undefined && true === params) {
42+
if (window === undefined && window === undefined && params === true) {
4643
ONE_FUNCTION_TO_BRING_THEM_ALL_AND_IN_THE_DARKNESS_BIND_THEM('a', 'b');
4744
const deprecatedImport = require('path/to/legacyModule');
4845
deprecatedImport.execute();
4946
deprecatedFunction();
50-
functionWithoutJSDocWarningsBecauseTheSectionWasCompletelyExcluded();
47+
shortFunction();
5148
$.each();
49+
return;
5250
debugger;// Make sure we get in here to set the value correctly
5351
}
52+
53+
/* We value sonarjs rules enough to test them, here. Sorry for the mess. */
54+
55+
function shortFunction (arg) {
56+
if (arg) {
57+
console.log(true);
58+
}
59+
return true;
60+
}
61+
62+
// sonarjs/no-identical-functions checks function bodies of three lines and above
63+
function duplicateFunction (arg) {
64+
if (arg) {
65+
console.log(true);
66+
}
67+
return true;
68+
}
69+
70+
// sonarjs/no-all-duplicated-branches
71+
if (true) {
72+
shortFunction();
73+
} else {
74+
shortFunction();
75+
}
76+
77+
// sonarjs/max-switch-cases, sonarjs/no-duplicated-branches
78+
switch (1) {
79+
case 1:
80+
break;
81+
case 2:
82+
switch (2) {
83+
case 1:
84+
duplicateFunction();
85+
duplicateFunction();
86+
break;
87+
case 1:
88+
duplicateFunction();
89+
duplicateFunction();
90+
break;
91+
}
92+
break;
93+
case 3:
94+
break;
95+
case 4:
96+
break;
97+
case 5:
98+
break;
99+
case 6:
100+
break;
101+
case 7:
102+
break;
103+
case 8:
104+
break;
105+
case 9:
106+
break;
107+
case 10:
108+
break;
109+
case 11:
110+
break;
111+
default:
112+
break;
113+
}

0 commit comments

Comments
 (0)