Skip to content

Conversation

@Trott
Copy link
Member

@Trott Trott commented Apr 7, 2018

  • Prefer "an argument is coerced to" over "an argument coerces to".
  • Reduce informal tone.
  • Wrap at 80 chars.
  • Wrap value in backticks where appropriate for consistency.
Checklist

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Apr 7, 2018
@Trott
Copy link
Member Author

Trott commented Apr 7, 2018

@Trott
Copy link
Member Author

Trott commented Apr 7, 2018

@nodejs/documentation

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 7, 2018
@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 7, 2018
Copy link
Member

@addaleax addaleax Apr 7, 2018

Choose a reason for hiding this comment

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

nit: I’d say something like If `value` is a string, this encoding is used to look it up in the buffer., or something else that doesn’t sound like the encoding is a inherent to the string value itself

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax Maybe this? If `value` is a string, this is the encoding used to convert `value` to a binary representation before searching for `value` in `buf`?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm...except my wording possibly sounds like value is mutated...

Copy link
Member

Choose a reason for hiding this comment

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

@Trott Yeah, good point. How about: If `value` is a string, this function looks for the representation of `value` in `encoding`.? It’s less ambiguous and arguably captures the semantics better, but it might also be harder to understand what “representation” means here?

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM with a nit.

Copy link
Member

@ChALkeR ChALkeR Apr 7, 2018

Choose a reason for hiding this comment

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

In the first part of the sentance, this looks a little less clear to me with the «coerced» word.

Perhaps, «For arguments that coerce to … the entire buffer will be searched» or something similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

byteOffset is the only argument here for which the statement is applicable so maybe this?

If byteOffset is not a number, it will be coerced to a number. If it is
coerced to NaN or 0, then the entire buffer will be searched. This

...of this?:

If byteOffset is not a number, it will be coerced to a number. If the result
of coercion is NaN or 0, then the entire buffer will be searched. This

Copy link
Member

Choose a reason for hiding this comment

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

The second one looks perfect!

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

+1 for:

If byteOffset is not a number, it will be coerced to a number. If the result
of coercion is NaN or 0, then the entire buffer will be searched. This

* improve usage of "coerce" in buffer.md
* reduce informal tone in buffer.md
* wrap line at 80 characters in buffer.md
* wrap value in backtick for consistency
@Trott
Copy link
Member Author

Trott commented Apr 9, 2018

Trott added a commit to Trott/io.js that referenced this pull request Apr 9, 2018
* improve usage of "coerce" in buffer.md
* reduce informal tone in buffer.md
* wrap line at 80 characters in buffer.md
* wrap value in backtick for consistency

PR-URL: nodejs#19861
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 9, 2018

Landed in 28e5c46

@Trott Trott closed this Apr 9, 2018
targos pushed a commit that referenced this pull request Apr 12, 2018
* improve usage of "coerce" in buffer.md
* reduce informal tone in buffer.md
* wrap line at 80 characters in buffer.md
* wrap value in backtick for consistency

PR-URL: #19861
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
* improve usage of "coerce" in buffer.md
* reduce informal tone in buffer.md
* wrap line at 80 characters in buffer.md
* wrap value in backtick for consistency

PR-URL: nodejs#19861
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Trott Trott deleted the coerce branch January 13, 2022 22:49
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. buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants