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 the exception description #3658

Closed
wants to merge 2 commits into from
Closed

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented Nov 4, 2015

A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class Error.

Just read the new documentation of Node.js, and raise this up :-)

/cc @chrisdickinson

A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.
@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Nov 4, 2015
@Fishrock123
Copy link
Contributor

@nodejs/documentation

@Trott
Copy link
Member

Trott commented Nov 4, 2015

I would prefer changing ...it is not required that these values inherit from... to ...it is not required that exceptions inherit from... and leave it at that. Here's why:

  • I'm not sure that introducing the word class doesn't cause more problems in this context than the word inherit.
  • This usage is pretty typical anyway and I'm not sure that there is much to be gained by getting too hung up on it. This MDN doc includes p is an object that inherits from o and so on and so forth and it just doesn't seem problematic.
  • I find the original shorter version easier to read and understand than the currently proposed longer version. This is a way to eliminate the value objection but also keep the sentence short and easy to understand.

@yorkie
Copy link
Contributor Author

yorkie commented Nov 5, 2015

I'm not sure that introducing the word class doesn't cause more problems in this context than the word inherit.

The class has been introduced by ES6 itself, so I don't think we could keep skipping this problem.

This usage is pretty typical anyway and I'm not sure that there is much to be gained by getting too hung up on it. This MDN doc includes p is an object that inherits from o and so on and so forth and it just doesn't seem problematic.

Even thought MDN and wikipedia specify the inheritance is for an object or class, but for future JavaScript usage, the inheritance would look like:

class Foo extends Bar {
  ...
}

And we cannot do like this:

(new Foo) extends Bar;
// syntax error

So, I think to keep the consistence with ES6's syntax, we should change the description to: inheritance should only on classes.

I find the original shorter version easier to read and understand than the currently proposed longer version. This is a way to eliminate the value objection but also keep the sentence short and easy to understand.

100% agreed on changing from value to that exceptions, it as well should be more clear :-)

@yorkie
Copy link
Contributor Author

yorkie commented Nov 26, 2015

Closing because of inactivity, thank you guys

@yorkie yorkie closed this Nov 26, 2015
@yorkie yorkie deleted the fix/doc branch November 26, 2015 12:24
@Trott
Copy link
Member

Trott commented Nov 26, 2015

If you wish to re-open, the last version you had LGTM.

@yorkie
Copy link
Contributor Author

yorkie commented Nov 26, 2015

Really? I thought you rejected it :-(

@Trott
Copy link
Member

Trott commented Nov 26, 2015

I had some comments, and you addressed them. I thought it was fine. Sorry I didn't make that clear! Maybe let's see if we can get one more person's opinion? /cc @nodejs/documentation

@yorkie yorkie restored the fix/doc branch November 26, 2015 17:59
@yorkie
Copy link
Contributor Author

yorkie commented Nov 26, 2015

Lol, never mind, REOPEN-ing, thank you :-)

@yorkie yorkie reopened this Nov 26, 2015
@@ -243,8 +243,9 @@ by other contexts.
<!--type=misc-->

A JavaScript "exception" is a value that is thrown as a result of an invalid operation or
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The scare quotes around exception are unnecessary IMO. Since we're editing nearby text, maybe this is a good opportunity to remove them. Totally optional, feel free to ignore, I'm fine with the change as is.

@yorkie
Copy link
Contributor Author

yorkie commented Nov 26, 2015

@Trott Agreed and fixed :-)

@Trott
Copy link
Member

Trott commented Nov 26, 2015

LGTM. Unless someone else has an objection, I'll land this on Monday UTC. (It's a major holiday in the U.S. today, so I want to give some time for people to chime in, since the issue was dormant for so long. Thanks for your patience!)

@evanlucas
Copy link
Contributor

LGTM

@Qard
Copy link
Member

Qard commented Nov 26, 2015

LGTM too. 👍

@cjihrig
Copy link
Contributor

cjihrig commented Nov 26, 2015

LGTM

@Trott
Copy link
Member

Trott commented Dec 1, 2015

Landed in 2fb34b6

@Trott Trott closed this Dec 1, 2015
Trott pushed a commit that referenced this pull request Dec 1, 2015
A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.

PR-URL: #3658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@yorkie yorkie deleted the fix/doc branch December 1, 2015 03:20
rvagg pushed a commit that referenced this pull request Dec 5, 2015
A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.

PR-URL: #3658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@rvagg rvagg mentioned this pull request Dec 8, 2015
MylesBorins pushed a commit that referenced this pull request Dec 15, 2015
A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.

PR-URL: #3658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.

PR-URL: #3658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.

PR-URL: #3658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
A value shouldn't be described as doing inherit from some class, more
strictly, the value is an instance of the class `Error`.

PR-URL: nodejs#3658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Stephan Belanger <admin@stephenbelanger.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants