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

Added enforced linting #436

Merged
merged 7 commits into from
Jun 1, 2021
Merged

Added enforced linting #436

merged 7 commits into from
Jun 1, 2021

Conversation

willarmiros
Copy link
Contributor

Issue #, if available:
#329

Description of changes:
Added a top-level .eslintrc.json which contains all config, and just extended from it in all subpackages. We use the @typescript-eslint/recommended and import/recommended base configurations, which don't end up being too opinionated, but is in line with what the AWS SDK uses. I considered adding Prettier, and am still not opposed, but neither the AWS SDK nor OpenTelemetry JS uses it (see open-telemetry/opentelemetry-js#2204 for more on the recent removal).

It turns out I can't really add a linting rule to prevent an import like the one that caused #427 because it's a perfectly valid import, but such issues should be caught by the smoke tests now anyway and the repo was long overdue for linting.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@willarmiros willarmiros requested a review from anuraaga May 28, 2021 00:13
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

Merging #436 (a5897a9) into master (b6f18c6) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
- Coverage   82.71%   82.70%   -0.01%     
==========================================
  Files          36       36              
  Lines        1741     1740       -1     
==========================================
- Hits         1440     1439       -1     
  Misses        301      301              
Impacted Files Coverage Δ
lib/segments/attributes/captured_exception.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6f18c6...a5897a9. Read the comment docs.

@willarmiros
Copy link
Contributor Author

Looks like there's some weirdness with the hapi package linting. Will address before merging.

.eslintrc.json Outdated Show resolved Hide resolved
@@ -55,7 +55,7 @@ var RuleCache = {
if(v !== 0) return v;
if(a.getName() > b.getName())
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect our lint to enforce braces around blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, will add the curly rule

William Armiros and others added 3 commits June 1, 2021 11:32
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
@willarmiros willarmiros merged commit a9d0cf9 into aws:master Jun 1, 2021
@willarmiros willarmiros deleted the linting branch June 1, 2021 21:06
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.

3 participants