Review requested:

• @nodejs/crypto

While it technically reduces the number of statements, I don't really see how this improves the code or its readability.

While it technically reduces the number of statements, I don't really see how this improves the code or its readability.

I see the concern, but grouping returning values in a single case reduces redundancy and improves code efficiency, making it easier to maintain and read.

Even though it's a minor repetition, this constitutes a code duplication and violates the DRY (Don't Repeat Yourself) principle. As a result, you can already observe instances of the same operation being performed in various parts of the project. Despite the small magnitude of the issue, I still strive to contribute to the project in any way possible.

https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L49
https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L110
https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L155
https://github.com/nodejs/node/blob/main/lib/internal/crypto/cfrg.js#L249
https://github.com/nodejs/node/blob/main/lib/internal/crypto/ec.js#L258
https://github.com/nodejs/node/blob/main/lib/internal/crypto/mac.js#L73

And more...

If this is going to fallthrough would you mind moving the returns on newlines to actually help readability?

If this is going to fallthrough would you mind moving the returns on newlines to actually help readability?

Thank you for your response, I have rearranged the code as you suggested.

- Validating Jenkins credentials
✘  Jenkins credentials invalid

Need to wait until https://nodejs.org/en/blog/vulnerability/august-2023-security-releases is through to start CI and let this progress.

It is a cosmetic change. I'm approving only because there is another approval. With two approvals we can land it faster without waiting one week.

I see the concern, but grouping returning values in a single case reduces redundancy and improves code efficiency, making it easier to maintain and read.

I am not sure if it really improves code efficiency, and as @panva said, even if it does, efficiency really doesn't matter much here.

I don't think this is an improvement because intuitively this is a one-to-one mapping from algorithms to block sizes. One line per algorithm, the name on the left and the corresponding value on the right. Easily readable and mathematically concise.

Grouping algorithms like this makes no sense. The fact that SHA-1 and SHA-256 have the same internal block size is purely coincidental. The algorithms are vastly different.

If we keep adding algorithms to the list, this change will effectively demand ordering algorithms by their block size. For example, the SHA-3 bitrates (i.e., block sizes) fall both below and above that of SHA-512, so I guess we'd have to mix SHA-2 and SHA-3 in this list?

Anyway, I won't object, but I don't think these changes help.

Anyway, I won't object, but I don't think these changes help.

Same here.

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

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

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

FWIW, the commit message still doesn't adhere to the commit message guidelines since it doesn't begin with an imperative verb after the prefix.

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

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

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

Landed in 2b57e47