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

feat: lint examples #649

Merged
merged 46 commits into from
Jan 24, 2020
Merged

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Dec 26, 2019

Which problem is this PR solving?

Fixes #603

Short description of the changes

Adds .eslintrc at root of examples folder, lints said examples (WIP) and adds a step in pipeline to validate that the examples are linted.

@codecov-io
Copy link

codecov-io commented Dec 26, 2019

Codecov Report

Merging #649 into master will increase coverage by 1.75%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   90.92%   92.67%   +1.75%     
==========================================
  Files         224      227       +3     
  Lines       10256    10337      +81     
  Branches      962      940      -22     
==========================================
+ Hits         9325     9580     +255     
+ Misses        931      757     -174
Impacted Files Coverage Δ
...s/opentelemetry-web/test/StackScopeManager.test.ts 0% <0%> (ø) ⬆️
...ackages/opentelemetry-web/src/WebTracerRegistry.ts 100% <0%> (ø) ⬆️
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ages/opentelemetry-plugin-http/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0%> (-9.53%) ⬇️
packages/opentelemetry-plugin-mysql/src/utils.ts 90.9% <0%> (-4.55%) ⬇️
packages/opentelemetry-web/test/WebTracer.test.ts 0% <0%> (ø) ⬆️
packages/opentelemetry-web/src/utils.ts 94.73% <0%> (ø) ⬆️
...ry-scope-async-hooks/src/AsyncHooksScopeManager.ts 97.67% <0%> (ø) ⬆️
...try-propagator-jaeger/src/JaegerHttpTraceFormat.ts 95.65% <0%> (ø) ⬆️
... and 181 more

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Each example has to have its own .eslintrc? is there no way to share a single one in the whole examples folder?

@naseemkullah
Copy link
Member Author

Each example has to have its own .eslintrc? is there no way to share a single one in the whole examples folder?

Sounds sensible, will look into that.
BTW airbnb tends to remove 'use strict;', do we care enough to override that rule? Should we try another eslint-config?

@dyladan
Copy link
Member

dyladan commented Dec 26, 2019

Each example has to have its own .eslintrc? is there no way to share a single one in the whole examples folder?

Sounds sensible, will look into that.
BTW airbnb tends to remove 'use strict;', do we care enough to override that rule? Should we try another eslint-config?

I used the airbnb rules on a previous project and found them to be generally sensible, but one of the overrides we applied was to always use strict. The airbnb style guide assumes you are using the airbnb development toolchain which inserts use strict during the compile step :) https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/strict.js#L3

@dyladan
Copy link
Member

dyladan commented Dec 26, 2019

@naseemkullah it looks like one of the rules is no-use-before-defined whose behavior I don't happen to like. JS has function hoisting which I find makes code a lot easier to understand. I like to have the "main" function at the top, with the utility functions it calls at the bottom, which reads much more naturally to me. With the airbnb rules you are required to scroll past all the utility functions before you get to the function that actually does the work.

furthermore, override the strict rule that ships with airbnb
examples/.eslintrc Outdated Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented Dec 26, 2019

Can you add a yarn script lint:examples to the root package.json? Some people may not have eslint installed globally.

Also add the dev dependencies.

@naseemkullah
Copy link
Member Author

naseemkullah commented Dec 26, 2019

Can you add a yarn script lint:examples to the root package.json? Some people may not have eslint installed globally.

Sure sounds good,
Why do some scripts separate words with : (e.g. codecov:browser) and others with - (e.g. docs-deploy) ? What's the convention here?

@dyladan
Copy link
Member

dyladan commented Dec 26, 2019

Can you add a yarn script lint:examples to the root package.json? Some people may not have eslint installed globally.

Sure sounds good,
Why do some scripts separate words with : (e.g. codecov:browser) and others with - (e.g. docs-deploy) ? What's the convention here?

no convention afaik. just different people adding them

@naseemkullah
Copy link
Member Author

naseemkullah commented Jan 14, 2020

@dyladan I've fixed conflicts and I've added many exceptions as there was too much non easily lintable code. Lets please get this in before more changes are made to examples to avoid fixing conflicts.

@naseemkullah
Copy link
Member Author

bump

@naseemkullah naseemkullah requested a review from dyladan January 18, 2020 19:55
package.json Outdated Show resolved Hide resolved
Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

LGTM

@OlivierAlbertini OlivierAlbertini added enhancement New feature or request size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 21, 2020
@mayurkale22
Copy link
Member

I think this is good to go, right? /cc @dyladan

@dyladan
Copy link
Member

dyladan commented Jan 24, 2020

@mayurkale22 yes I think so. I didn't want to merge without your ✅

@dyladan dyladan merged commit d7f4fe2 into open-telemetry:master Jan 24, 2020
@naseemkullah naseemkullah deleted the lint-examples branch April 9, 2021 18:18
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linting "/examples"
5 participants