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

feat: add support for win32 paths #402

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

davakh
Copy link

@davakh davakh commented Feb 7, 2024

I implemented general logic for resolving win32 relative paths in library. It's hard to say what else to cover with tests because it's hard to fully mock Node.js's path module and easily switch between win32/posix paths resolving in jest.

If you have thoughts on what else should I cover, I would like to hear about it and I can improve coverage if it's required.

fixes #401

Copy link

linux-foundation-easycla bot commented Feb 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thank you looks good, can we add more test with real examples?

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: Patch coverage is 61.11111% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 92.18%. Comparing base (58464fc) to head (a070b9a).
Report is 20 commits behind head on main.

Files Patch % Lines
lib/util/path.js 35.48% 15 Missing and 5 partials ⚠️
lib/SelfReferencePlugin.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
- Coverage   92.85%   92.18%   -0.67%     
==========================================
  Files          43       44       +1     
  Lines        2042     2085      +43     
  Branches      598      619      +21     
==========================================
+ Hits         1896     1922      +26     
- Misses        118      131      +13     
- Partials       28       32       +4     

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

@alexander-akait
Copy link
Member

Also please fix ts problems, thank you

@alexander-akait
Copy link
Member

@davakh Hello, friendly ping ⭐

@davakh
Copy link
Author

davakh commented Mar 6, 2024

Hello, sorry that it took so long. In new version, I've added tests and rewritten the original path tests based on the fact that this repository has separate runners for Windows. Also, I'm still not sure what you mean by "real examples" and want to ask for a hint about that, but I've added platform-based tests to the main resolve function, so it should cover more cases.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

There are enough tests. Thank you

@davakh
Copy link
Author

davakh commented Mar 7, 2024

I'll take a look on failed tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Merge
Development

Successfully merging this pull request may close these issues.

Add ability to handle win32 relative paths
3 participants