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

lib: .load .save add proper error message when no file passed #52225

Merged

Conversation

thomas-mauran
Copy link
Contributor

This commit adds a proper error message using ERR_MISSING_ARGS('file') when a .save or .load REPL command is runned. This commit also adds test for both of this cases.

Fixes: #52218

This commit adds a proper error message using ERR_MISSING_ARGS('file')
when a .save or .load REPL command is runned. This commit also adds
test for both of this cases.

Fixes: nodejs#52218

Signed-off-by: Thomas Mauran <[email protected]>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Mar 27, 2024
@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 30, 2024
@nodejs-github-bot

This comment was marked as outdated.

@cola119
Copy link
Member

cola119 commented Mar 30, 2024

@thomas-mauran Thank you for your contribution! The changes look good to me, but the linters have flagged some style issues. Could you please address them?
https://github.com/nodejs/node/actions/runs/8448275081/job/23257872321?pr=52225

Signed-off-by: Thomas Mauran <[email protected]>
@thomas-mauran
Copy link
Contributor Author

Hello @cola119 I applied the lint correction I think the test will pass now

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@cola119 cola119 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@thomas-mauran
Copy link
Contributor Author

Hello @cola119 and @mertcanaltin I'm not sure of what should I do for the failed test ?

@nodejs-github-bot
Copy link
Collaborator

@cola119
Copy link
Member

cola119 commented Mar 31, 2024

They are flaky tests, so I re-run the CI.

@thomas-mauran
Copy link
Contributor Author

still some didn't pass @cola119 idk if it's because of the flaky test ? my pr shouldn't affect them

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@cola119
Copy link
Member

cola119 commented Apr 2, 2024

Retrying CI...

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 3, 2024

This comment was marked as outdated.

@panva panva added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@cola119 cola119 added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Apr 5, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 5, 2024
@nodejs-github-bot

This comment was marked as outdated.

@cola119 cola119 added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 5, 2024
@thomas-mauran
Copy link
Contributor Author

nice test seems to have passed ! thank you @cola119

@cola119 cola119 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 6, 2024
@nodejs-github-bot nodejs-github-bot merged commit 3f5ff8d into nodejs:main Apr 6, 2024
74 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3f5ff8d

@cola119
Copy link
Member

cola119 commented Apr 6, 2024

@thomas-mauran Thank you for your contribution! I apologize for some flaky tests that prolonged the time it took for our CI to pass. I'm looking forward to your continued contributions in the future!

@thomas-mauran
Copy link
Contributor Author

no problem at all thanks for taking the time !

marco-ippolito pushed a commit that referenced this pull request May 2, 2024
This commit adds a proper error message using ERR_MISSING_ARGS('file')
when a .save or .load REPL command is runned. This commit also adds
test for both of this cases.

Fixes: #52218

Signed-off-by: Thomas Mauran <[email protected]>
PR-URL: #52225
Reviewed-By: Kohei Ueno <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
This commit adds a proper error message using ERR_MISSING_ARGS('file')
when a .save or .load REPL command is runned. This commit also adds
test for both of this cases.

Fixes: #52218

Signed-off-by: Thomas Mauran <[email protected]>
PR-URL: #52225
Reviewed-By: Kohei Ueno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPL special commands need arg validation
5 participants