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

src: consolidate exit codes in the code base #44746

Merged
merged 6 commits into from
Oct 6, 2022

Conversation

joyeecheung
Copy link
Member

Add an ExitCode enum class and use it throughout the code base instead of hard-coding the exit codes everywhere. At the moment, the exit codes used in many places do not actually conform to what the documentation describes. With the new enums (which are also available to the JS land as constants in an internal binding) we could migrate to a more consistent usage of the codes, and eventually expose the constants to the user land when they are stable enough.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 22, 2022
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I think this is a neat idea! A few nits:

lib/internal/process/promises.js Outdated Show resolved Hide resolved
lib/internal/main/watch_mode.js Outdated Show resolved Hide resolved
lib/internal/debugger/inspect.js Outdated Show resolved Hide resolved
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Oh sweet! Thanks for this.

src/node.cc Outdated Show resolved Hide resolved
src/env.cc Show resolved Hide resolved
Add an ExitCode enum class and use it throughout the code base
instead of hard-coding the exit codes everywhere. At the moment,
the exit codes used in many places do not actually conform to
what the documentation describes. With the new enums (which
are also available to the JS land as constants in an internal
binding) we could migrate to a more consistent usage of the
codes, and eventually expose the constants to the user land
when they are stable enough.
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Failing tests seem relevant.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Fixed the Windows failure @jasnell @Trott @aduh95 @JakobJingleheimer can you take a look again? Thanks!

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

👍 I can't speak to the C change aside from I'm fairly sure it's innocuous, and the fix is pretty clearly working.

src/node_exit_code.h Show resolved Hide resolved
src/node_exit_code.h Show resolved Hide resolved
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 6, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2022
@nodejs-github-bot nodejs-github-bot merged commit be525d7 into nodejs:main Oct 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in be525d7

@danielleadams
Copy link
Contributor

@joyeecheung do you mind backporting this to v18.x? It landed with conflicts.

@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Oct 11, 2022
@joyeecheung
Copy link
Member Author

From the results of git node backport preferably these commits should be backported first

I'll see if I can prepare a PR with these

debadree25 added a commit to debadree25/node that referenced this pull request Dec 13, 2022
debadree25 added a commit to debadree25/node that referenced this pull request Jan 3, 2023
debadree25 added a commit to debadree25/node that referenced this pull request Feb 19, 2023
nodejs-github-bot pushed a commit that referenced this pull request Feb 26, 2023
Refs: #44746
PR-URL: #45841
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants