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.

Sign up for GitHub

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

Jump to bottom

fix(lint): move tslint to eslint #892

Merged
merged 37 commits into from
May 21, 2020
Merged

fix(lint): move tslint to eslint #892

merged 37 commits into from
May 21, 2020

Conversation

OlivierAlbertini
Copy link
Member

@OlivierAlbertini OlivierAlbertini commented Mar 23, 2020 •
edited
Loading

Which problem is this PR solving?

I will check #1083
Because .prettierrc.json through gts changes a lot of files (from 175 to 328).
I have an issue with circleci config where it doesn't see the .prettierrc.json (but locally yes).
It doesn't fix this comment #892 (comment) even if we add .prettierrc.json file through gts (gts and prettier have this rule by default)

@dyladan
Copy link
Member

dyladan commented Apr 11, 2020

GTS version released. Can you update this and get it ready to go out?

@dyladan dyladan mentioned this pull request Apr 11, 2020
Closed
@OlivierAlbertini OlivierAlbertini force-pushed the feature/tslint-eslint branch 8 times, most recently from f5cf5bf to 7214b5c Compare April 15, 2020 21:54
@OlivierAlbertini
Copy link
Member Author

GTS version released. Can you update this and get it ready to go out?

yes, I would like to validate https://github.com/open-telemetry/opentelemetry-js/blob/7214b5c64a4d8c9fd40456360df1faca92ff88ae/eslint.rules.js with you before going further

I tweaked the circleci file in order to show you that everything is fine, I will finalize the circleci once all packages will be migrated.

@OlivierAlbertini OlivierAlbertini force-pushed the feature/tslint-eslint branch 2 times, most recently from c4b31dc to 8cec4b6 Compare April 15, 2020 23:21
@codecov-io
Copy link

codecov-io commented Apr 15, 2020 •
edited by codecov bot
Loading

Codecov Report

Merging #892 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #892   +/-   ##
=======================================
  Coverage   94.97%   94.97%           
=======================================
  Files         235      235           
  Lines        9081     9081           
  Branches      821      821           
=======================================
  Hits         8625     8625           
  Misses        456      456

dyladan
dyladan reviewed Apr 16, 2020
View reviewed changes
packages/opentelemetry-plugin-http/src/utils.ts Outdated Show resolved Hide resolved
eslint.rules.js Outdated Show resolved Hide resolved
eslint.rules.js Outdated Show resolved Hide resolved
eslint.rules.js Outdated Show resolved Hide resolved
eslint.rules.js Outdated Show resolved Hide resolved
@OlivierAlbertini
Copy link
Member Author

I'm starting other packages

@dyladan
Copy link
Member

dyladan commented Apr 16, 2020

Wait on that. Other plugins will be moving to the contrib repo. There is already a PR open for that #969

@OlivierAlbertini OlivierAlbertini force-pushed the feature/tslint-eslint branch 3 times, most recently from 1fc51c7 to 808d4a7 Compare April 17, 2020 17:44
@OlivierAlbertini OlivierAlbertini marked this pull request as ready for review April 17, 2020 17:56
dyladan
dyladan reviewed Apr 17, 2020
View reviewed changes
packages/opentelemetry-api/src/metrics/Metric.ts Show resolved Hide resolved
packages/opentelemetry-base/src/ExportResult.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-zone-peer-dep/src/ZoneContextManager.ts Show resolved Hide resolved
packages/opentelemetry-exporter-collector/src/CollectorExporter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-metrics/src/export/ConsoleMetricExporter.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-grpc/src/grpc.ts
@@ -135,15 +134,13 @@ export class GrpcPlugin extends BasePlugin<grpc> {
plugin._logger.debug('patched gRPC server');

return function register<RequestType, ResponseType>(
// tslint:disable-next-line:no-any
Copy link
Member

Choose a reason for hiding this comment

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

Would like to enable the no-any rule, even if we have to put ignores in some places. I try to avoid the use of any wherever possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

When you lint your code, you have warnings... and it shows lines, files and the rule you don't respect. It's less noisy.

packages/opentelemetry-plugin-http/test/utils/httpRequest.ts Outdated Show resolved Hide resolved
packages/opentelemetry-resources/src/platform/node/detectors/EnvDetector.ts Show resolved Hide resolved
packages/opentelemetry-test-utils/testUtils.ts Show resolved Hide resolved
OlivierAlbertini referenced this pull request in VilledeMontreal/opentelemetry-js Apr 18, 2020
@OlivierAlbertini
fix(eslint): addressed Dyladan comment
cda08a5 
https://github.com/open-telemetry/opentelemetry-js/pull/892\#discussion_r410517397
@OlivierAlbertini
feat(metrics): move to eslint
054f433
@OlivierAlbertini
feat(node): move to eslint
043843f
@OlivierAlbertini
feat(propagator-jaeger): move to eslint
ab353c2
@OlivierAlbertini
feat(resources): move to eslint
c8b2e2e
@OlivierAlbertini
feat(shim-opentracing): move to eslint
2800cbe
@OlivierAlbertini
feat: move other packages to eslint
dc2cda0
@OlivierAlbertini
fix: remove comments
86a0bc7
@OlivierAlbertini
feat(xml-http-request): move to eslint
1d31816
@OlivierAlbertini
fix(exporter-collector): karma test
e54852b
@OlivierAlbertini
fix: remove tslint references
d540594
@OlivierAlbertini
fix(eslint): addressed Dyladan comment
8083d1c 
https://github.com/open-telemetry/opentelemetry-js/pull/892\#discussion_r410517397
@OlivierAlbertini
fix: add return line to .eslintignore
fcbf46c
@OlivierAlbertini
fix(eslint): addressed Dyladan comment
c96d005 
https://github.com/open-telemetry/opentelemetry-js/pull/892\#discussion_r410703274
@OlivierAlbertini
fix(eslint): revert change in http test
8511874
@OlivierAlbertini
fix(eslint): disable ban-ts-ignore only for tests
dad37af
@OlivierAlbertini
fix: addressed mayurkale22 comments
a466291
@OlivierAlbertini
fix(eslint): lint fix since rebase
ed63a14
@OlivierAlbertini
fix: addressed obecny comment
cc01f4d
@OlivierAlbertini OlivierAlbertini force-pushed the feature/tslint-eslint branch 2 times, most recently from 7077419 to cc01f4d Compare May 20, 2020 02:23
@dyladan
Copy link
Member

dyladan commented May 20, 2020

Looks like your build is failed

@mayurkale22 mayurkale22 merged commit 05e7bb9 into open-telemetry:master May 21, 2020
@legendecas legendecas mentioned this pull request May 21, 2020
Closed
@mayurkale22 mayurkale22 mentioned this pull request May 21, 2020
Closed
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
@dyladan
fix(lint): move tslint to eslint (open-telemetry#892)
f001e85
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
feat: amqplib instrumentation (open-telemetry#892)
f6c16b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dyladan dyladan dyladan approved these changes

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@markwolff markwolff markwolff approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@obecny obecny obecny approved these changes

@bg451 bg451 Awaiting requested review from bg451

@rochdev rochdev Awaiting requested review from rochdev

@mwear mwear Awaiting requested review from mwear

Assignees
No one assigned
Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Moving from tslint to eslint
7 participants
@OlivierAlbertini @dyladan @codecov-io @obecny @vmarchaud @markwolff @mayurkale22