Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eslint migration #567

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Eslint migration #567

merged 2 commits into from
Dec 21, 2021

Conversation

stephen-slm
Copy link
Contributor

@stephen-slm stephen-slm commented Nov 19, 2021

Eslint Upgrade #287

This pull request focuses on the migration from semistandard to eslint with standard/standard as the direct rule set for linting the application. By moving to eslint allows future development and migration to Typescript smoother. While also supporting linting, code style enforcement, and support of locating errors.

eslint: https://eslint.org/

Linted Content

Linting has been run throughout the application which has mainly shifted var to const and var to let where applicable. While also correcting string index references to direct references e.g a['test'] to a.test.

Node V8 Support (Breaking Change)

Development of the project will no longer support Node version 8 due to eslint using unsupported JavaScript features. Node V8 is also currently deprecated since 2019-12-31 and this should not be seen as a negative breaking change.

This was referenced Nov 19, 2021
@coveralls
Copy link

coveralls commented Nov 19, 2021

Coverage Status

Coverage remained the same at 86.427% when pulling 9af4205 on stephensli:eslint-migration into f239ac6 on ethereum:master.

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look fine to me. The only thing to settle is the node.js v8 support.

Personally, I think it's fine drop it - even v10 is already EOL and the major Solidity frameworks that depend on solc-js already require v12+ themselves. I'd like to get an ack from @chriseth and/or @axic before we go forward with it though.

Also, if we're dropping it, please adjust .circleci/config.yml to remove the job that runs on that version. I'd remove the disabled v4 and v6 as well. And I'd also add a comment explaining that ESLint is the reason behind the v10+ requirement.

@stephen-slm stephen-slm requested a review from cameel December 6, 2021 20:28
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm marking it as approved since I don't see any problem with dropping v8 and I suspect that @chriseth and @axic will agree. We still need to wait for their approval though - especially because the node-v8 job is marked as a required check and I don't have permissions to remove it from repo settings myself.

I think you can already safely rebase your other PR on this one though.

@cameel
Copy link
Member

cameel commented Dec 9, 2021

#566 (comment) was actually meant to go here:

I asked @axic about the PR on the call today. He's going to take a look but one problem he pointed out already is that removal of v8 support is technically a breaking change unfortunately.

Well, maybe we could make a breaking 0.9.x release early?

@cameel cameel mentioned this pull request Dec 14, 2021
@cameel
Copy link
Member

cameel commented Dec 14, 2021

@axic Reposting this from the chat so that it does not get lost:

This is what I managed to find so far for projects we have listed in #10278

https://en.wikipedia.org/wiki/Node.js#Releases

 8.x    End-of-Life      Carbon       2017-05-30   2019-12-31
 9.x    End-of-Life                   2017-10-01   2018-06-30
10.x    End-of-Life      Dubnium      2018-04-24   2021-04-30
11.x    End-of-Life                   2018-10-23   2019-06-01
12.x    Maintenance LTS  Erbium       2019-04-23   2022-04-30
13.x    End-of-Life                   2019-10-22   2020-06-01
14.x    Active LTS       Fermium      2020-04-21   2023-04-30
15.x    End of Life                   2020-10-20   2021-06-01
16.x    Active LTS       Gallium      2021-04-20   2024-04-30
17.x    Current                       2021-10-19   2022-06-01
18.x    Planned                       2022-04-19   2025-04-30 

@cameel
Copy link
Member

cameel commented Dec 20, 2021

@chriseth approved dropping v8 support. This should now be rebased on #572.

Stephen Lineker-Miller added 2 commits December 21, 2021 11:14
Migrating to eslint allows using the mainstream highly maintained linting
tool that will directly support linting, code style enforcement and
support of locating errors.

 * Directly supports integrating with Typescript for future proofing
 * Supports enabling semi colon enforcement via rules
Linting has been ran throughout the application which has mainly shifted
`var` to `const` and `var` to `let` where applicable. While also correcting
string index references to direct references e.g a['test'] to a.test.
@stephen-slm
Copy link
Contributor Author

@cameel done

@chriseth chriseth merged commit d40c3f4 into ethereum:master Dec 21, 2021
@stephen-slm stephen-slm deleted the eslint-migration branch December 21, 2021 11:41
@axic
Copy link
Member

axic commented Jan 23, 2022

Why did this miss a bunch of other issues, like single-quotes vs double-quotes, missing semicolons, spacing around function names and object, etc. ?

cc @stephensli @cameel

@stephen-slm
Copy link
Contributor Author

@axic you are going to have to provide examples, a quick fly over is all good.

The only file that was not linted was: solcjs due to the extension not existing. #566 makes this change.

@axic
Copy link
Member

axic commented Jan 24, 2022

solcjs as you mentioned due to the filename issue, but also test/compiler.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants