Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: normalize packageManager field when parsing #184

Merged
merged 22 commits into from
Jan 27, 2025

Conversation

BobbieGoede
Copy link
Contributor

@BobbieGoede BobbieGoede commented Jan 22, 2025

Resolves #176

This removes the pattern ^\W+ to drop unexpected characters preceding the package manager name (and version), while still allowing unknown name patterns \w+.

If the pattern removal results in a different package name a warning will be returned and logged when used in the CLI detect command, the warning can be disabled by passing the --warn false.

Notes

  • The sanitizePackageManagerName returns any for the sanitized name, this matches the original behavior, but gives a false sense of type safety, perhaps something to look into in a separate PR.

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 64.83%. Comparing base (660392f) to head (9df3188).
Report is 78 commits behind head on main.

Files with missing lines Patch % Lines
src/cli.ts 0.00% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #184       +/-   ##
===========================================
- Coverage   82.17%   64.83%   -17.34%     
===========================================
  Files           6        5        -1     
  Lines         516      583       +67     
  Branches       71      113       +42     
===========================================
- Hits          424      378       -46     
- Misses         91      202      +111     
- Partials        1        3        +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BobbieGoede
Copy link
Contributor Author

I'll look into adding a simple unit test for the function

@BobbieGoede
Copy link
Contributor Author

@pi0
I have renamed the function and moved it to _utils.ts to keep it encapsulated while being able to import it for tests and added basic tests.

Warnings are now added to an optional warnings property on PackageManager, maybe a bit odd for one warning but will be less intrusive if more warnings are added in the future.

@BobbieGoede BobbieGoede requested a review from pi0 January 24, 2025 11:28
@BobbieGoede
Copy link
Contributor Author

BobbieGoede commented Jan 24, 2025

@pi0
Hmm I don't quite follow, should we use the previous /^\W+/ replacement instead of matching /\w+$/?

For consistency we could use warnings btw.

So we want the pattern to be a warnings string array when applicable?

@BobbieGoede BobbieGoede requested a review from pi0 January 27, 2025 09:19
@pi0 pi0 changed the title feat: sanitize detected packageManager name feat: parse and normalize packageManager field Jan 27, 2025
@pi0
Copy link
Member

pi0 commented Jan 27, 2025

Thanks for the PR. worked on it a little bit more. If the name part matches the npm package name regex, we parse it as-is, otherwise, we fall back to sanitize+warn mode.

I also added buildMeta with the new parser util to be exposed.

@pi0 pi0 changed the title feat: parse and normalize packageManager field feat: normalize packageManager field when parsing Jan 27, 2025
@pi0 pi0 merged commit 246b61a into unjs:main Jan 27, 2025
5 of 6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanitize packageManager for detection
2 participants