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

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

Merged
merged 2 commits into from
Apr 3, 2020
Merged

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.

const FIPS_ENABLED = 1;
const FIPS_DISABLED = 0;

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.

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.

💯

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!

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

PR-URL: nodejs#32580
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
@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
`crypto.getFips()` returns a number, not a boolean.

PR-URL: #32580
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
`crypto.getFips()` returns a number, not a boolean.

PR-URL: #32580
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Apr 22, 2020
`crypto.getFips()` returns a number, not a boolean.

PR-URL: #32580
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[email protected]>
targos pushed a commit that referenced this pull request Apr 22, 2020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Signed-off-by: Richard Lau <[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. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants