Skip to content
This repository was archived by the owner on Jun 16, 2025. It is now read-only.

Conversation

dobesv
Copy link
Contributor

@dobesv dobesv commented Jun 1, 2022

  • Allow string array return
  • Use Record instead of mapped type.
  • Remove content parameter - this doesn't seem to ever be passed in

Allow string array return, use `Record` instead of mapped type.
Based on the code, file content is not passed into parsers.
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2022

Codecov Report

Merging #718 (53129e9) into main (73c2bec) will decrease coverage by 0.19%.
The diff coverage is 84.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
- Coverage   98.64%   98.45%   -0.18%     
==========================================
  Files          49       49              
  Lines        1023     1031       +8     
==========================================
+ Hits         1009     1015       +6     
- Misses         14       16       +2     
Impacted Files Coverage Δ
src/utils/configuration-reader.js 100.00% <ø> (ø)
src/cli.js 95.56% <75.00%> (-4.44%) ⬇️
src/detector/importDeclaration.js 100.00% <100.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 3d1a45f...53129e9. Read the comment docs.

@rumpl
Copy link
Member

rumpl commented Jun 1, 2022

Hm ok, so win CI is failing with a nice error message

gyp ERR! stack Error: Could not find any Visual Studio installation to use

I'm quite sure it's not related to your changes.


type Parser = (content: string, filePath: string, deps: ReadonlyArray<string>, rootDir: string) => Node;
type Parser = (filePath: string, deps: ReadonlyArray<string>, rootDir: string) => Node | string[];
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this change, the parser doesn't have the content parameter? Seems weird to change the type definition and not the code at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the function is called here:

https://github.com/depcheck/depcheck/blob/main/src/check.js#L51

And it only passes in the latter three parameters. I guess these types have just been incorrect since this change:

5735ee7

The implementation was updated there but not the types.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Ok 😅

@rumpl
Copy link
Member

rumpl commented Jun 1, 2022

Ok so the win build is just broken for some obscure Visual Studio reason, I'll just merge this one and take a look at the windows build error...

@rumpl rumpl merged commit 3776553 into depcheck:main Jun 1, 2022
@dobesv dobesv deleted the patch-1 branch June 1, 2022 19:39
@jtbandes
Copy link
Contributor

Hi, is there any plan to publish this in a release?

@rumpl
Copy link
Member

rumpl commented Nov 15, 2022

Hi @jtbandes I'll try and push one this week 👍

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

Successfully merging this pull request may close these issues.

4 participants