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

Update for crypto.md #21500

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@WispProxy
Copy link
Contributor

commented Jun 24, 2018

Description:

  • crypto.randomFillSync - update for JS example
  • crypto.scrypt, crypto.scryptSync - update for options description
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Update for crypto.md
crypto.randomFillSync - update for JS example
crypto.scrypt, crypto.scryptSync - update for options description
@vsemozhetbyt
Copy link
Contributor

left a comment

Thank you! Just some tiny nits.

@@ -2149,12 +2149,10 @@ added: v10.5.0
- `salt` {string|Buffer|TypedArray}
- `keylen` {number}
- `options` {Object}
- `N` {number} CPU/memory cost parameter. Must be a power of two greater
than one. **Default:** `16384`.
- `N` {number} CPU/memory cost parameter. Must be a power of two greater than one. **Default:** `16384`.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 24, 2018

Contributor

Please, wrap long lines at 80 characters or less. Just align with the first list item character, like this:

  - `N` {number} CPU/memory cost parameter. Must be a power of two greater than
    one. **Default:** `16384`.
- `r` {number} Block size parameter. **Default:** `8`.
- `p` {number} Parallelization parameter. **Default:** `1`.
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*r > maxmem` **Default:** `32 * 1024 * 1024`.
- `maxmem` {number} Memory upper bound. It is an error when (approximately) `128*N*r > maxmem` **Default:** `32 * 1024 * 1024`.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 24, 2018

Contributor

The same)

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 24, 2018

Contributor

Maybe `128*N*r -> `128 * N * r for better readability?

@@ -2195,12 +2193,10 @@ added: v10.5.0
- `salt` {string|Buffer|TypedArray}
- `keylen` {number}
- `options` {Object}
- `N` {number} CPU/memory cost parameter. Must be a power of two greater
than one. **Default:** `16384`.
- `N` {number} CPU/memory cost parameter. Must be a power of two greater than one. **Default:** `16384`.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 24, 2018

Contributor

The same re wrapping.

- `r` {number} Block size parameter. **Default:** `8`.
- `p` {number} Parallelization parameter. **Default:** `1`.
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*r > maxmem` **Default:** `32 * 1024 * 1024`.
- `maxmem` {number} Memory upper bound. It is an error when (approximately) `128*N*r > maxmem` **Default:** `32 * 1024 * 1024`.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 24, 2018

Contributor

The same re wrapping and spaces.

@vsemozhetbyt vsemozhetbyt referenced this pull request Jun 25, 2018

Closed

crypto: add better scrypt option aliases #21525

4 of 4 tasks complete
Update crypto.md
crypto.scrypt, crypto.scryptSync - update for formatting
@WispProxy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Done. Please check it.

@@ -2150,11 +2150,11 @@ added: v10.5.0
- `keylen` {number}
- `options` {Object}
- `N` {number} CPU/memory cost parameter. Must be a power of two greater
than one. **Default:** `16384`.
than one. **Default:** `16384`.

This comment has been minimized.

Copy link
@tniessen

tniessen Jun 26, 2018

Member

Do we prefer this style @vsemozhetbyt? Seems like the current style causes issues when converted to HTML?

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 26, 2018

Contributor

Yeah, the extra indentation produces false code blocks in HTML (not sure if this is per Markdown spec or just a quirk of marked we use in md-2-HTML conversion). See in the last nightly docs:

https://nodejs.org/download/nightly/v11.0.0-nightly201806268836a0d780/docs/api/crypto.html#crypto_crypto_scrypt_password_salt_keylen_options_callback

@vsemozhetbyt
Copy link
Contributor

left a comment

Just two nits that can be addressed on landing if it is not convenient to fix them now)

- `r` {number} Block size parameter. **Default:** `8`.
- `p` {number} Parallelization parameter. **Default:** `1`.
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*r > maxmem` **Default:** `32 * 1024 * 1024`.
`128 * N * r > maxmem` **Default:** `32 * 1024 * 1024`.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 26, 2018

Contributor

Sorry for not spotting this first time: it seems we lack the period between maxmem` and **Default:**. Can you add it since we are already here?)

This comment has been minimized.

Copy link
@WispProxy

WispProxy Jun 26, 2018

Author Contributor

@vsemozhetbyt "it seems we lack the period between maxmem and **Default:**" What do you mean on "period" word? Could you please write an example?

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 26, 2018

Contributor

Sorry for the ambiguity) I mean we need this fix:

maxmem`. **Default:**

This comment has been minimized.

Copy link
@WispProxy

WispProxy Jun 26, 2018

Author Contributor

I see. Sure.

- `r` {number} Block size parameter. **Default:** `8`.
- `p` {number} Parallelization parameter. **Default:** `1`.
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*r > maxmem` **Default:** `32 * 1024 * 1024`.
`128 * N * r > maxmem` **Default:** `32 * 1024 * 1024`.

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 26, 2018

Contributor

Ditto)

Update crypto.md
crypto.scrypt, crypto.scryptSync - added dot symbol
@WispProxy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Please re-check it.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

vsemozhetbyt added a commit that referenced this pull request Jun 27, 2018

doc: fix code example and formatting in crypto.md
PR-URL: #21500
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Landed in 02fd93d
Thank you!

@WispProxy WispProxy deleted the WispProxy:patch-1 branch Jun 28, 2018

targos added a commit that referenced this pull request Jun 28, 2018

doc: fix code example and formatting in crypto.md
PR-URL: #21500
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@targos targos referenced this pull request Jul 3, 2018

Merged

v10.6.0 proposal #21629

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.