Fix linter-identified issues and run the linter before tests #250
Conversation
| 'browser': true, | ||
| 'worker': true, | ||
| 'node': true | ||
| extends: 'airbnb-base', |
There was a problem hiding this comment.
The airbnb-base config doesn't require the React and JSX plugins. Since those were unused in this project, they are removed from the dependencies below.
There was a problem hiding this comment.
That makes sense. I was not aware of the base config, but this is exactly what we need.
| @@ -30,16 +37,17 @@ module.exports = { | |||
| 'no-console': 0, | |||
| 'no-bitwise': 0, | |||
| 'max-classes-per-file': 0, | |||
| 'max-len': ['error', { code: 130 }], | |||
There was a problem hiding this comment.
The other changes in this file are not real configuration changes (only removing unnecessary quotes to satisfy the linter). This is the one rule configuration change. Since there are multiple instances of lines longer than 100 chars, I thought it made sense to relax this rule to allow this.
| "eslint-plugin-react": "^7.19.0", | ||
| "eslint": "^7.32.0", | ||
| "eslint-config-airbnb-base": "^14.2.1", | ||
| "eslint-plugin-import": "^2.24.2", |
There was a problem hiding this comment.
The eslint-plugin-react and eslint-plugin-jsx-a11y dependencies are no longer needed (not required by the AirBnB base config).
| "prepare": "npm run build", | ||
| "pretest": "npm run lint", |
There was a problem hiding this comment.
The lint task will now run before npm test.
| @@ -18,7 +18,7 @@ export default class LercDecoder extends BaseDecoder { | |||
| case LercAddCompression.None: | |||
| break; | |||
| case LercAddCompression.Deflate: | |||
| buffer = inflate(new Uint8Array(buffer)).buffer; | |||
| buffer = inflate(new Uint8Array(buffer)).buffer; // eslint-disable-line no-param-reassign, prefer-destructuring | |||
There was a problem hiding this comment.
If prefer-destructuring is not ignored, this would need to be updated to be
({ buffer } = inflate(new Uint8Array(buffer)));
| @@ -83,18 +83,20 @@ export default class DataSlice { | |||
| const right = this.readUint32(offset + 4); | |||
| let combined; | |||
| if (this._littleEndian) { | |||
| combined = left + 2 ** 32 * right; | |||
| combined = left + ((2 ** 32) * right); | |||
There was a problem hiding this comment.
The no-mixed-operators rule wants explicit grouping here and below so people don't have to remember precedence rules.
| @@ -46,7 +45,7 @@ class RemoteSource extends BaseSource { | |||
| } | |||
|
|
|||
| // otherwise make a single request for each slice | |||
| return await Promise.all( | |||
| return Promise.all( | |||
There was a problem hiding this comment.
It may not be obvious without expanding to get additional context, but this is the return of an async function. So returning a promise is the same as returning the resolved value.
There was a problem hiding this comment.
Yep, this was bugging me for long now.
| import isNode from 'detect-node'; | ||
| import 'isomorphic-fetch'; | ||
| import { expect } from 'chai'; | ||
|
|
||
| import { makeFetchSource } from '../src/geotiff'; | ||
| import { makeFetchSource } from '../src/source/remote'; |
There was a problem hiding this comment.
The '../src/geotiff.js' module does not export makeFetchSource.
It looks like the tests have not run test/source.js since #131 (in that change, npm test was made to run only test/geotiff.spec.js). I'm not sure if that was intentional.
There was a problem hiding this comment.
Hm, I believe not. Have to take a look at this. Thanks!
| 'browser': true, | ||
| 'worker': true, | ||
| 'node': true | ||
| extends: 'airbnb-base', |
There was a problem hiding this comment.
That makes sense. I was not aware of the base config, but this is exactly what we need.
| @@ -46,7 +45,7 @@ class RemoteSource extends BaseSource { | |||
| } | |||
|
|
|||
| // otherwise make a single request for each slice | |||
| return await Promise.all( | |||
| return Promise.all( | |||
There was a problem hiding this comment.
Yep, this was bugging me for long now.
| import isNode from 'detect-node'; | ||
| import 'isomorphic-fetch'; | ||
| import { expect } from 'chai'; | ||
|
|
||
| import { makeFetchSource } from '../src/geotiff'; | ||
| import { makeFetchSource } from '../src/source/remote'; |
There was a problem hiding this comment.
Hm, I believe not. Have to take a look at this. Thanks!
Since the repo already has an ESLint configuration, I thought it could make sense to fix the linter-identified issues. This branch also makes it so the linter is run as a
pretestscript. I know this is a big, risky-looking change, and I understand if you don't feel like accepting it. I'll leave more comments on the specifics of some of the changes below.