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

fix(opentelemetry-sdk-node): move nock to dev dependencies #2219

Merged
merged 2 commits into from
May 24, 2021

Conversation

nflaig
Copy link
Contributor

@nflaig nflaig commented May 21, 2021

This PR moves nock from dependencies to devDependencies since the library is only required during development.

Signed-off-by: nflaig <nflaig@protonmail.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 21, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

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

Out of curiosity did you just happen to notice this or is there a tool that can produce a warning about this?

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #2219 (9c9307a) into main (f13b35f) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 9c9307a differs from pull request most recent head 960e3c2. Consider uploading reports for the commit 960e3c2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2219      +/-   ##
==========================================
- Coverage   92.71%   92.69%   -0.02%     
==========================================
  Files         142      142              
  Lines        5120     5120              
  Branches     1049     1049              
==========================================
- Hits         4747     4746       -1     
- Misses        373      374       +1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@nflaig
Copy link
Contributor Author

nflaig commented May 21, 2021

@johnbley I happend to notice it while reviewing the code but npm-check could potentially be used to do this in an automated way, for example

cp packages/opentelemetry-sdk-node/package.json packages/opentelemetry-sdk-node/src/package.json
npm-check packages/opentelemetry-sdk-node/src --production
rm packages/opentelemetry-sdk-node/src/package.json

would produce the following

nock   😎  MAJOR UP  Major update available. https://github.com/nock/nock#readme
                    npm install --save nock@13.0.11 to go from 12.0.3 to 13.0.11
       😕  NOTUSED?  Still using nock?
                    Depcheck did not find code similar to require('nock') or import from 'nock'.
                    Check your code before removing as depcheck isn't able to foresee all ways dependencies can be used.
                    Use --skip-unused to skip this check.
                    To remove this package: npm uninstall --save nock

This does not seem to be the most robust solution I guess but it would have worked in this specific case.

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.

lgtm, nice finding

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

Successfully merging this pull request may close these issues.

7 participants