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 configuration for getting-started examples #2297

Merged
merged 9 commits into from
Aug 23, 2021

Conversation

alisabzevari
Copy link
Contributor

fixes #1390

Which problem is this PR solving?

  • Adds linting to getting-started directory.

Short description of the changes

  • I tried to use the similar configuration as examples directory. Although the ts-example needed another configuration.
  • I have added two lint scripts to the pacakge.json but they are not automatically being executed before the commit. This is the case for examples as well.

@alisabzevari alisabzevari changed the title Getting started eslint eslint configuration for getting-started examples Jun 23, 2021
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #2297 (54dc78a) into main (78a78c0) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2297      +/-   ##
==========================================
+ Coverage   92.70%   92.72%   +0.02%     
==========================================
  Files         137      137              
  Lines        4990     4990              
  Branches     1054     1054              
==========================================
+ Hits         4626     4627       +1     
+ Misses        364      363       -1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

@alisabzevari
Copy link
Contributor Author

Seems like the test is flaky. How can I retrigger the tests?

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

why did you remove use strict ?

@alisabzevari
Copy link
Contributor Author

alisabzevari commented Jun 26, 2021

eslint disallows strict mode directives if the sourceType is module (Docs).

@alisabzevari alisabzevari force-pushed the getting-started-eslint branch from ddbf67f to 832866d Compare June 26, 2021 10:05
@dyladan
Copy link
Member

dyladan commented Jun 30, 2021

The examples are not ecmascript modules so the sourceType for at least the examples should be script

@alisabzevari alisabzevari force-pushed the getting-started-eslint branch 2 times, most recently from 672c547 to 878bee2 Compare July 4, 2021 18:32
@alisabzevari alisabzevari requested a review from obecny July 8, 2021 07:43
@dyladan
Copy link
Member

dyladan commented Jul 26, 2021

Is there any way this can use the already existing eslint rules we have? Don't want to have to maintain two different sets of rules.

@alisabzevari alisabzevari requested a review from rauno56 as a code owner August 3, 2021 13:25
@alisabzevari alisabzevari force-pushed the getting-started-eslint branch from f0b144c to 632ed1c Compare August 3, 2021 13:28
@alisabzevari
Copy link
Contributor Author

Is there any way this can use the already existing eslint rules we have?

I changed the eslintrc to import the eslintrc from the examples directory.

@alisabzevari alisabzevari force-pushed the getting-started-eslint branch from 632ed1c to a286de2 Compare August 3, 2021 13:34
@dyladan dyladan removed the internal label Aug 3, 2021
@dyladan dyladan added the document Documentation-related label Aug 3, 2021
package.json Show resolved Hide resolved
getting-started/ts-example/.eslintrc Show resolved Hide resolved
@@ -0,0 +1,16 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

in general, why can't you use our eslint.config.js from main folder and extend from it ?

Copy link
Member

Choose a reason for hiding this comment

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

That's what he did above https://github.com/open-telemetry/opentelemetry-js/pull/2297/files#diff-4bb0b44ed2e9b719ea2593f63cf9aebd302f9e7a1f3a109e5c35f6ee59d95d87R4

I think he probably just forgot to apply the suggestion to both eslintrc files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to use the project eslint config for typescript files because it requires that getting-started being added to the projects. I get this error when using the project eslint config:

0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: getting-started/ts-example/traced-example/tracing.ts.
The file must be included in at least one of the projects provided

@alisabzevari alisabzevari requested a review from a team August 8, 2021 15:46
@alisabzevari alisabzevari force-pushed the getting-started-eslint branch from e274636 to a034441 Compare August 8, 2021 15:54
@obecny obecny merged commit c62ea11 into open-telemetry:main Aug 23, 2021
@dyladan dyladan mentioned this pull request Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint getting started guide
4 participants