Feature eslint fix violations #577
Conversation
|
Checks need to pass before this can be considered for merging. |
angel4576
force-pushed
the
feature-eslint-fix
branch
from
81255c4
to
ed7105b
Compare
…into feature-eslint-fix
There was a problem hiding this comment.
I went through one not very large file (3dmol.js) and found at least two breaking changes. This does not inspire confidence. I think perhaps large scale changes like this should wait until there is greater code coverage with the tests. It does not help that there appear to be some changes made that aren't required by eslint, which just results in more diffs to review.
In any case, eslint should be run as part of the build process. There are currently
✖ 19676 problems (11196 errors, 8480 warnings)
This PR cannot be merged until those are zero.
I'd recommend reducing the scope of the linting to just the eslint recommended rules, or an even smaller subset (e.g. disable the suggestions). Once the system is integrated into the build it will be easy enough to incrementally enable rules.
|
|
||
| return my; | ||
| // eslint-disable-next-line no-var | ||
| var $3Dmol = (function threeDeeMollContructor(window) { |
There was a problem hiding this comment.
why? use => if it is going to complain
| })(window); | ||
|
|
||
| if ( typeof module === "object" && typeof module.exports === "object" ) { |
There was a problem hiding this comment.
why triple equals to double equals?
There was a problem hiding this comment.
Hello professor, the option for eqeqeq rule is set to "smart", which enforces the use of === and !== except for comparing two literal values, evaluating typeof values, and comparing with null.
But triple equals should also work
| /** | ||
| * Create and initialize an appropriate viewer at supplied HTML element using specification in config | ||
| @function $3Dmol.createViewer | ||
| * @param {Object | string} element - Either HTML element or string identifier | ||
| * @param {ViewerSpec} config Viewer configuration | ||
| * @param {Object} shared_viewer_resources shared resources between viewers' renderers |
There was a problem hiding this comment.
we can't change the API, probably best to disable the camelcase rule
| * @return [[$3Dmol.GLViewer]] 2D array of GLViewers | ||
| * @example | ||
| var viewers = $3Dmol.createViewerGrid( | ||
| 'gldiv', //id of div to create canvas in | ||
| { | ||
| rows: 2, | ||
| cols: 2, | ||
| control_all: true //mouse controls all viewers |
| const massage = val => { | ||
| if ($3Dmol.isNumeric(val)) { | ||
| // hexadecimal does not parse as float | ||
| if (Math.floor(parseFloat(val, 16)) === parseInt(val, 16)) { |
There was a problem hiding this comment.
parseFloat doesn't take a radix. This changes the behavior to not be right. We are relying on parseInt to figure out the form of the number (e.g. "0xa" should return 10). If radix must be explicit, set it to zero.
| return parseFloat(val); // ".7" for example, does not parseInt | ||
| } | ||
|
|
||
| return parseInt(val, 10); |
| ysum += atom.y; | ||
| zsum += atom.z; | ||
|
|
||
| xmin = xmin < atom.x ? xmin : atom.x; |
There was a problem hiding this comment.
what eslint rule is requiring clarifying parentheses be removed? as far as I can tell, it doesn't mind so why were these changes made?
| element = typeof element == 'string' ? $(`#${element}`) : element; | ||
| if (!element) return; | ||
|
|
||
| const viewers = $3Dmol.createViewerGrid(element, {rows: 1, cols: 2, controlAll: true}); |
|
Using a linter like ESLint is an excellent idea in my opinion and I highly appreciate @angel4576 effort to introduce a consistent style to the project. At the same time, I understand @dkoes 's concern that other API and behavioral changes were introduced in this PR. As projects typically benefit from a consistent code style and adherence to best practices, I encourage the use of a linter. One could consider restarting this PR - maybe with a more minimalist linter config. As @dkoes said, one can always make the linter stricter or more pedantic later on. What are your thoughts? |
|
@RyanGuild is in the middle of a massive refactor that will convert all of 3Dmol.js to typescript with an updated build system that includes linting. Hence all the merge conflicts. |
|
Exciting, thank you for your efforts. |
|
@dkoes By when we can expect to see the new Typescript change? Interested to contribute. |
|
That is an excellent question. Unfortunately, @RyanGuild seems to have dropped off the face of the earth half way through the project and this is a crazy busy semester for me. 😢 |
|
I see. Maybe I can go through the typescript branches created by @RyanGuild and see what I can do 🤔 |
|
That would be fantastic! |
|
It looks like, he was trying to add rollup first but then he stopped. Can be seen in feature-modules branch. Later on, he decided to use Parcel as a bundler. Some of the branches which are not merged in the master are (Correct me if I'm wrong) - I think we need to first see what all changes were made in these branches and whether these are even needed or not. Too much work! 😬 |
Description
Fix all eslint and prettier violations. #567
Type of change
How Has This Been Tested?
npm run buildMy Configuration:
npm --version: 8.3.1node --version: v16.14.0python3 --version: 3.10.0