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.

Sign up for GitHub

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

Jump to bottom

feat: add fs/resolve-parent-paths-by #2897

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

feat: add fs/resolve-parent-paths-by #2897

wants to merge 15 commits into from
+3,767 −0

Conversation

gururaj1512
Copy link
Member

@gururaj1512 gururaj1512 commented Sep 12, 2024 •
edited
Loading

Resolves #2567.

Description

What is the purpose of this pull request?
Adds JS implementation for @stdlib/fs/resolve-parent-paths-by/

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@stdlib-js/reviewers

@gururaj1512 gururaj1512 changed the title resolve-parent-paths-by feat: add @stdlib/fs/resolve-parent-paths-by Sep 12, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Review A pull request which needs code review. Utilities Issue or pull request concerning general utilities. labels Sep 16, 2024
@gururaj1512
Copy link
Member Author

hey @kgryte, can you explain this linting error in README file at line 3.
Screenshot 2024-09-16 165059

@Snehil-Shah
Copy link
Member

@gururaj1512 is this when you try to commit?

@gururaj1512
Copy link
Member Author

@Snehil-Shah that error occurs when commiting and also while running eslint

@Snehil-Shah
Copy link
Member

@gururaj1512 Weird. I can't seem to reproduce the error. Did you follow the setup guide properly? (make install, make init). Maybe try clean installing and setting up again (after make clean).
Also, can you show the full error and what exactly are/were you trying to commit?

@gururaj1512
Copy link
Member Author

@gururaj1512 Weird. I can't seem to reproduce the error. Did you follow the setup guide properly? (make install, make init). Maybe try clean installing and setting up again (after make clean). Also, can you show the full error and what exactly are/were you trying to commit?

Yaa, I have tried make clean & make install
Lint Error
This error occurs while running command npx eslint path/to/directory/

@Snehil-Shah
Copy link
Member

@gururaj1512 Weird. I can't seem to reproduce the error. Did you follow the setup guide properly? (make install, make init). Maybe try clean installing and setting up again (after make clean). Also, can you show the full error and what exactly are/were you trying to commit?

Yaa, I have tried make clean & make install Lint Error This error occurs while running command npx eslint path/to/directory/

Ok, was able to reproduce it. Seems to me, it's something specific to npx as I am able to commit the file without any errors. And in any case, you don't need to run npx eslint manually, the linter is automatically triggered before every commit via git hooks (assuming your environment is set up as expected). If you still wish to manually trigger the linter, you can use the make recipes (like make lint) listed here.

Snehil-Shah
Snehil-Shah reviewed Sep 22, 2024
View reviewed changes
Copy link
Member

@Snehil-Shah Snehil-Shah left a comment

Choose a reason for hiding this comment

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

LGTM otherwise!

Snehil-Shah
Snehil-Shah suggested changes Sep 22, 2024
View reviewed changes
Copy link
Member

@Snehil-Shah Snehil-Shah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also, was the issue with your dev environment resolved @gururaj1512?

@gururaj1512
Copy link
Member Author

Also, was the issue with your dev environment resolved @gururaj1512?

@Snehil-Shah, the issue with my dev environment has been resolved. Thank you for checking in!

Snehil-Shah
Snehil-Shah approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@Snehil-Shah Snehil-Shah left a comment

Choose a reason for hiding this comment

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

LGTM. On to @kgryte.

@Snehil-Shah Snehil-Shah requested a review from kgryte September 24, 2024 10:25
@kgryte kgryte changed the title feat: add @stdlib/fs/resolve-parent-paths-by feat: add fs/resolve-parent-paths-by Sep 24, 2024
kgryte
kgryte reviewed Sep 24, 2024
View reviewed changes
lib/node_modules/@stdlib/fs/resolve-parent-paths-by/lib/main.js Outdated
*/
function onExists( error, bool ) { // eslint-disable-line node/handle-callback-err
if ( bool ) {
test( spath, onTest );
Copy link
Member

Choose a reason for hiding this comment

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

Same problem here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Snehil-Shah, Can you also go through this function & rereview it.

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Sep 24, 2024
kgryte
kgryte requested changes Sep 24, 2024
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

This PR needs to address the race conditions during async processing before we can merge.

@gururaj1512
Copy link
Member Author

@kgryte, Modified the code to address race conditions. Can you re-review it.

@kgryte kgryte added the Needs Review A pull request which needs code review. label Nov 18, 2024
@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

/stdlib merge

@kgryte
Copy link
Member

kgryte commented Nov 25, 2024

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Nov 25, 2024
@kgryte kgryte removed the Needs Changes Pull request which needs changes before being merged. label Nov 25, 2024
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Nov 25, 2024
@stdlib-bot
Copy link
Contributor

Coverage Report

Package Statements Branches Functions Lines
fs/resolve-parent-paths-by $\color{red}923/952$
$\color{green}+96.95\%$ 		$\color{red}127/136$
$\color{green}+93.38\%$ 		$\color{green}26/26$
$\color{green}+100.00\%$ 		$\color{red}923/952$
$\color{green}+96.95\%$

The above coverage report was generated for the changes in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgryte kgryte kgryte requested changes

@Snehil-Shah Snehil-Shah Snehil-Shah approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Feature Issue or pull request for adding a new feature. Needs Review A pull request which needs code review. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: add @stdlib/fs/resolve-parent-paths-by
4 participants
@gururaj1512 @Snehil-Shah @kgryte @stdlib-bot