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

src: use ARES_SUCCESS instead of 0 #48834

Merged
merged 1 commit into from
Jul 23, 2023
Merged

src: use ARES_SUCCESS instead of 0 #48834

merged 1 commit into from
Jul 23, 2023

Conversation

illusi0n7
Copy link
Contributor

Since error messages are also using defined values, it is more clear to use the defined value even when the operation succeeds.

@illusi0n7
src: use ARES_SUCCESS instead of 0
7b4fa6e 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Jul 19, 2023
@ShogunPanda
Copy link
Contributor

Is ARES_SUCCESS defined as 0?

@illusi0n7
Copy link
Contributor Author

sure. like this.

// ares.h
#define ARES_SUCCESS 0

@ShogunPanda
Copy link
Contributor

I guess I'm fine with it then!

@illusi0n7
Copy link
Contributor Author

illusi0n7 commented Jul 19, 2023 •
edited
Loading

And return value is compared with ARES_SUCCESS in cares_wrap.h

//cares_wrap.h
status = Traits::Parse(this, response_data_);
if (status != ARES_SUCCESS)
  ParseError(status);

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 19, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52859/

@github-actions github-actions bot mentioned this pull request Jul 20, 2023
Open
22 tasks
@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2023
@github-actions github-actions bot mentioned this pull request Jul 21, 2023
Open
16 tasks
This was referenced Jul 22, 2023
Open
Open
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 23, 2023
@nodejs-github-bot nodejs-github-bot merged commit 841b29c into nodejs:main Jul 23, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 841b29c

This was referenced Jul 24, 2023
Open
Open
Open
@github-actions github-actions bot mentioned this pull request Jul 27, 2023
Open
18 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
@illusi0n7 @Ceres6
src: use ARES_SUCCESS instead of 0
693aff9 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
@illusi0n7
src: use ARES_SUCCESS instead of 0
4af966c 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
@illusi0n7
src: use ARES_SUCCESS instead of 0
993e493 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
@pluris pluris mentioned this pull request Aug 7, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@illusi0n7 @Ceres6
src: use ARES_SUCCESS instead of 0
6ef2899 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@illusi0n7 @Ceres6
src: use ARES_SUCCESS instead of 0
2611b2c 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@illusi0n7 @UlisesGascon
src: use ARES_SUCCESS instead of 0
eb300a5 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@illusi0n7 @RafaelGSS
src: use ARES_SUCCESS instead of 0
578c3d1 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: #48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Aug 20, 2023
src: use ARES_SUCCESS instead of 0
982e7a6 
PR-URL: #49048
Refs: #48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@UlisesGascon
src: use ARES_SUCCESS instead of 0
f2552a4 
PR-URL: #49048
Refs: #48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
@illusi0n7 @targos
src: use ARES_SUCCESS instead of 0
67dba57 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: #48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
@targos
src: use ARES_SUCCESS instead of 0
158c91a 
PR-URL: #49048
Refs: #48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
src: use ARES_SUCCESS instead of 0
d43a85e 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs/node#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
src: use ARES_SUCCESS instead of 0
534aa0e 
PR-URL: nodejs/node#49048
Refs: nodejs/node#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
src: use ARES_SUCCESS instead of 0
208d856 
Since error messages are also using defined values, it is more clear
to use the defined value even when the operation succeeds.

PR-URL: nodejs/node#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
src: use ARES_SUCCESS instead of 0
59cf0e0 
PR-URL: nodejs/node#49048
Refs: nodejs/node#48834
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daeyeon daeyeon daeyeon approved these changes

@deokjinkim deokjinkim deokjinkim approved these changes

@JungMinu JungMinu JungMinu approved these changes

@lpinca lpinca lpinca approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@illusi0n7 @nodejs-github-bot @ShogunPanda @lpinca @JungMinu @deokjinkim @daeyeon