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: Use .prototype. for consistency #38032

Merged
merged 1 commit into from Apr 21, 2021
Merged

Conversation

VoltrexKeyva
Copy link
Member

@VoltrexKeyva VoltrexKeyva commented Apr 1, 2021

Most of the documentation uses the property accessor (.) or .prototype. instead of # to point out some methods or properties (e.g. Buffer.alloc()), there's some that uses (Buffer#alloc()), this is a small change but just for the sake of consistency.

@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 1, 2021
@mscdex
Copy link
Contributor

mscdex commented Apr 2, 2021

-1 I believe the reason for the # is that in some cases it makes more sense to refer to a Class method generally, especially when there is no (specific) instance to be referenced.

Changing the # to a . can make things more confusing. For example: changing Buffer#slice() to Buffer.slice() makes it appear as though there is a slice() available on the Buffer constructor, when it fact it's a prototype/instance method instead.

@VoltrexKeyva
Copy link
Member Author

-1 I believe the reason for the # is that in some cases it makes more sense to refer to a Class method generally, especially when there is no (specific) instance to be referenced.

Changing the # to a . can make things more confusing. For example: changing Buffer#slice() to Buffer.slice() makes it appear as though there is a slice() available on the Buffer constructor, when it fact it's a prototype/instance method instead.

I mean, it doesn't make much difference really, I don't see anyone thinking that # means something else other than a dot since in most projects if they're even used, they're considered as a dot, well notation really, but this could be improved by either puting the word prototype (e.g. Buffer.prototype.slice()), or it could be <Buffer>.slice(), but overall, yea; not really a bad point.

@DerekNonGeneric
Copy link
Contributor

I have never seen the hash notation to refer to instance methods, so it looked like a typo for me. I wonder if we have been consistent about doing this throughout all of our docs, though (doubtful). Using .prototype. instead of # doesn't really equate to representing an instance though (since the prototype is everpresent).

/cc @nodejs/documentation for more info, please

@VoltrexKeyva
Copy link
Member Author

I went through almost all of the documentation markdown files (/doc/api) and this file seems to be the only file that uses the # as notation for methods of class instances :suspect:

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Apr 7, 2021

I have never seen the hash notation to refer to instance methods, so it looked like a typo for me.

I just happened to come across the notation again in the @typescript-eslint/prefer-regexp-exec docs and can confirm that it is definitely not a typo, but an established pattern (altho I have no idea what it is called).

Use the RegExp#exec() method instead.

This suggestion expands to suggesting to use RegExp.prototype.exec, so here it is referring to the RegExp class' internal methods, but whether or not this can refer to any instance methods or not is still beyond me.

It would be nice to know what this notation is called, so we can determine exactly how to use if we do end up using it.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

Alright, after a bit of research, it has become clear to me that this change would be illogical since these are not static methods of these classes.

We would need one of the following changes to make it right:

  • typedArray.slice() - to make it look like an instance since they're not static (not a fan of this)
  • TypedArray.prototype.slice() - to make entirely less confusing, but a little more to read
  • TypedArray#slice() - to leave it as the shorter # notation

@ljharb
Copy link
Member

ljharb commented Apr 9, 2021

The dots are very much incorrect. Either TypedArray.prototype.slice or TypedArray#slice are the only ways they would be described in any documentation in the JS ecosystem.

@VoltrexKeyva
Copy link
Member Author

Alright, after a bit of research, it has become clear to me that this change would be illogical since these are not static methods of these classes.

We would need one of the following changes to make it right:

  • typedArray.slice() - to make it look like an instance they are not static (not a fan of this)
  • TypedArray.prototype.slice() - to make entirely less confusing, but a little more to read
  • TypedArray#slice() - to leave it as JavaScript foo.prototype.bar notation (apparently lol)

I think it would be better to show the instance properties/methods by adding .prototype. since it makes it a lot less confusing.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

Thanks for the speedy correction! We just need one more approval and we can get this landed.

Thanks!

@DerekNonGeneric
Copy link
Contributor

@VoltrexMaster, if you would be able to ensure that the line length does not exceed 80 characters now, it could improve the odds of getting more approvals with a green CI (Markdown linter is complaining).

@VoltrexKeyva
Copy link
Member Author

@VoltrexMaster, if you would be able to ensure that the line length does not exceed 80 characters now, it could improve the odds of getting more approvals with a green CI (Markdown linter is complaining).

👍 Done (Sometimes I hate linters lol)

@VoltrexKeyva VoltrexKeyva changed the title doc: Use dot for consistency doc: Use .prototype. for consistency Apr 9, 2021
@mscdex
Copy link
Contributor

mscdex commented Apr 10, 2021

I'd much rather opt for the TypedArray#slice() when referring to the instance method where there is no specific instance/context being discussed, otherwise typedArray.slice(). Including prototype not only makes it unnecessarily wordy IMO but I think it's less useful especially as the JS world moves closer towards the notion of (ES) classes (even if they're just syntactical sugar for prototypes behind the scenes) where prototypes aren't really discussed much.

@VoltrexKeyva
Copy link
Member Author

VoltrexKeyva commented Apr 10, 2021

I'd much rather opt for the TypedArray#slice() when referring to the instance method where there is no specific instance/context being discussed, otherwise typedArray.slice(). Including prototype not only makes it unnecessarily wordy IMO but I think it's less useful especially as the JS world moves closer towards the notion of (ES) classes (even if they're just syntactical sugar for prototypes behind the scenes) where prototypes aren't really discussed much.

The thing is, using # as the notation to reference instance properties/methods can confuse beginners or those who have never seen or encountered this style of referencing, besides that; # is not used in JavaScript except for declaring private properties/methods in classes which is still not how you reference them (e.g. this.#test()), and they're also private which can only be used inside the class itself which the properties/methods referenced here are not private, using # can confuse beginners which the documentation should be as beginner-friendly as possible.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I wish # notation had caught up more, but I agree it's the right call to use .prototype.:

  • in Node.js documentation, buffer.md is the only place where this notation is currently being used. I'm rooting for consistency.
  • MDN uses .prototype. notation, I think it makes sense to align with them.
  • prototype has a meaning for a reader proficient with ECMAScript, the # notation may seem out-of-place.

IMHO typedArray.slice() would not work here, the text is dealing with the effects of slice methods on a Buffer instance – which is also a TypedArray.

@marsonya marsonya added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 19, 2021
Most of the documentation uses `foo.prototype.bar` notation instead of
`foo#bar` notation, this commit apply the former in `buffer.md`.

PR-URL: nodejs#38032
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Apr 21, 2021

Landed in 8cb6b5d

@aduh95 aduh95 merged commit 8cb6b5d into nodejs:master Apr 21, 2021
@VoltrexKeyva VoltrexKeyva deleted the patch-1 branch April 22, 2021 06:01
targos pushed a commit that referenced this pull request Apr 29, 2021
Most of the documentation uses `foo.prototype.bar` notation instead of
`foo#bar` notation, this commit apply the former in `buffer.md`.

PR-URL: #38032
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targos targos mentioned this pull request May 3, 2021
targos pushed a commit that referenced this pull request May 30, 2021
Most of the documentation uses `foo.prototype.bar` notation instead of
`foo#bar` notation, this commit apply the former in `buffer.md`.

PR-URL: #38032
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
Most of the documentation uses `foo.prototype.bar` notation instead of
`foo#bar` notation, this commit apply the former in `buffer.md`.

PR-URL: #38032
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
Most of the documentation uses `foo.prototype.bar` notation instead of
`foo#bar` notation, this commit apply the former in `buffer.md`.

PR-URL: #38032
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet