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

doc: fix return type of crypto.getFips() #32580

Merged
merged 2 commits into from
Apr 3, 2020
Merged

doc: fix return type of crypto.getFips() #32580

merged 2 commits into from
Apr 3, 2020

Conversation

richardlau
Copy link
Member

crypto.getFips() returns a number, not a boolean.

cc @danbev @nodejs/crypto

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Mar 31, 2020
@richardlau
Copy link
Member Author

Matching the doc to the implementation seems like the least effort solution. Our tests for crypto.getFips() validate against numeric values, e.g.

node/test/parallel/test-crypto-fips.js

Lines 14 to 15 in c52ebc0

const FIPS_ENABLED = 1;
const FIPS_DISABLED = 0;

cjihrig
cjihrig approved these changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Optional nit: since there are many other possible values as a number (vs. a boolean) it might be worth noting the non-FIPS value now.

@richardlau
Copy link
Member Author

Optional nit: since there are many other possible values as a number (vs. a boolean) it might be worth noting the non-FIPS value now.

@cjihrig Added the non-FIPS value of 0 to the docs.

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 31, 2020
@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2020

Thanks. Still LGTM.

@tniessen
Copy link
Member

tniessen commented Apr 1, 2020

I assume it would be a breaking change to actually change it to a boolean?

@richardlau
Copy link
Member Author

I assume it would be a breaking change to actually change it to a boolean?

That was my assumption.

@tniessen
Copy link
Member

tniessen commented Apr 1, 2020

Technically, this LGTM as a documentation fix, I'm just concerned that by documenting the current behavior correctly, fixing the implementation to return a boolean becomes "more breaking".

@jasnell
Copy link
Member

jasnell commented Apr 1, 2020

"more breaking"

It would be semver-major either way so there's really no difference.

@addaleax
Copy link
Member

addaleax commented Apr 1, 2020

@jasnell That’s true as far as our release process is concerned – In terms of communicating with our users, there is a difference between changing behavior that’s documented vs changing behavior that’s undocumented.

What I would suggest for this PR is to document that in the future it may become a boolean, and then implement that in a follow-up at some point.

@richardlau
Copy link
Member Author

What I would suggest for this PR is to document that in the future it may become a boolean, and then implement that in a follow-up at some point.

Added more words to this effect.

addaleax
addaleax approved these changes Apr 2, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

💯

tniessen
tniessen approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

That's exactly what I mean, and a wonderful suggestion, @addaleax, thank you. And thanks for implementing it, @richardlau!

@richardlau
doc: fix return type of crypto.getFips()
d4fd03e 
`crypto.getFips()` returns a number, not a boolean.

PR-URL: nodejs#32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
@richardlau
doc: return type of crypto.getFips() may change
b9da063 
Document that the return type of `crypto.getFips()` may change in a
future semver-major release from a `number` to a `boolean`.

PR-URL: nodejs#32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
@richardlau richardlau merged commit b9da063 into nodejs:master Apr 3, 2020
@richardlau
Copy link
Member Author

Landed in 83ebd77...b9da063.

@richardlau richardlau deleted the getfips branch April 3, 2020 11:21
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@richardlau @BethGriggs
doc: fix return type of crypto.getFips()
8bfbb5e 
`crypto.getFips()` returns a number, not a boolean.

PR-URL: #32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@richardlau @BethGriggs
doc: return type of crypto.getFips() may change
d5da970 
Document that the return type of `crypto.getFips()` may change in a
future semver-major release from a `number` to a `boolean`.

PR-URL: #32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this pull request Apr 12, 2020
@richardlau @targos
doc: fix return type of crypto.getFips()
cf4f188 
`crypto.getFips()` returns a number, not a boolean.

PR-URL: #32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this pull request Apr 12, 2020
@richardlau @targos
doc: return type of crypto.getFips() may change
7f971b3 
Document that the return type of `crypto.getFips()` may change in a
future semver-major release from a `number` to a `boolean`.

PR-URL: #32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
@targos targos mentioned this pull request Apr 13, 2020
Merged
targos pushed a commit that referenced this pull request Apr 22, 2020
@richardlau @targos
doc: fix return type of crypto.getFips()
4604127 
`crypto.getFips()` returns a number, not a boolean.

PR-URL: #32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
@richardlau @targos
doc: return type of crypto.getFips() may change
a299e9c 
Document that the return type of `crypto.getFips()` may change in a
future semver-major release from a `number` to a `boolean`.

PR-URL: #32580
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>
@targos targos mentioned this pull request Apr 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen 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. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@richardlau @cjihrig @tniessen @jasnell @addaleax @lpinca @nodejs-github-bot