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

path: add another early exit to dirname #57058

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gurgunday
Copy link
Contributor

And adds another early exit to len === 2

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Feb 15, 2025
lib/path.js Outdated
const code = StringPrototypeCharCodeAt(path, 0);
if (len === 1) return isPathSeparator(code) ? path : '.';
if (len === 2 && isPathSeparator(StringPrototypeCharCodeAt(path, 1))) return path;
Copy link
Member

Choose a reason for hiding this comment

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

This is not equivalent to the original code. Let's wait for the CI

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I just read the description, please add a test case for this new behavior.

Copy link

codecov bot commented Feb 15, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (fd45383) to head (e46d565).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/path.js 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57058      +/-   ##
==========================================
+ Coverage   90.26%   90.27%   +0.01%     
==========================================
  Files         630      630              
  Lines      184634   184631       -3     
  Branches    36137    36130       -7     
==========================================
+ Hits       166654   166681      +27     
- Misses      11022    11023       +1     
+ Partials     6958     6927      -31     
Files with missing lines Coverage Δ
lib/path.js 96.08% <83.33%> (-0.01%) ⬇️

... and 37 files with indirect coverage changes

@lpinca lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Feb 15, 2025
@gurgunday
Copy link
Contributor Author

gurgunday commented Feb 21, 2025

@lpinca is it normal that it still hasn't got merged?

@lpinca
Copy link
Member

lpinca commented Feb 21, 2025

Yes, this needs more approvals or more wait time.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@nodejs-github-bot
Copy link
Collaborator

@gurgunday gurgunday changed the title path: (dirname) move early exits to the top path: add another early exit to dirname Feb 26, 2025
@gurgunday
Copy link
Contributor Author

Rebased and fixed commit message @lpinca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants