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 const consistently #4147

Closed
wants to merge 2 commits into from
Closed

doc: use const consistently #4147

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 4, 2015

events.markdown uses var and const in different places seemingly
arbitrarily. This change makes it favor const when possible.

@Trott Trott added the doc Issues and PRs related to the documentations. label Dec 4, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Dec 4, 2015

LGTM

1 similar comment
@JungMinu
Copy link
Member

JungMinu commented Dec 4, 2015

LGTM

@pmq20
Copy link
Contributor

pmq20 commented Dec 4, 2015

Why setting callback = function(stream) as a const? That does not seem very conventional.

@@ -164,7 +164,7 @@ Returns emitter, so calls can be chained.
Removes a listener from the listener array for the specified event.
**Caution**: changes array indices in the listener array behind the listener.

var callback = function(stream) {
const callback = function(stream) {
console.log('someone connected!');
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just use a function... definition? here. (I forget if that's the correct term or not...)

@jasnell
Copy link
Member

jasnell commented Dec 4, 2015

LGTM with @Fishrock123's suggestion

events.markdown uses `var` and `const` in different places seemingly
arbitrarily. This change makes it favor const when possible.
@Trott
Copy link
Member Author

Trott commented Dec 5, 2015

OK, changed that const callback = function(stream) to just function callback(stream). PTAL.

@silverwind
Copy link
Contributor

LGTM, but aren't there more cases of unnecessary var in the docs?

@silverwind
Copy link
Contributor

Did a quick search, lots of var in docs, which could possibly be made const but I don't really see a value in doing so as it'd likely be confusing to beginners. There's nothing inherently wrong with var.

In the same vein, one could say this change here is unnecessary, so I think I'll have to redact my LGTM.

@Trott
Copy link
Member Author

Trott commented Dec 5, 2015

@silverwind The reason why I did it for this doc in particular is that there were conflicting examples. Well, not really conflicting, but inconsistent. One used const and one used var for basically identical situations. That seemed to be potentially confusing to beginners so I went to weed out the needless inconsistency.

@Trott
Copy link
Member Author

Trott commented Dec 5, 2015

(I'd be just as happy to convert the existing const to a var for consistency, but it seems like the project tends to favor const where it can be used, so that's the direction I went instead.)

@Trott
Copy link
Member Author

Trott commented Dec 5, 2015

(One argument for var instead of const is that it will work outside of strict mode so we'd be making fewer assumptions about the user's environment.)

@Trott
Copy link
Member Author

Trott commented Dec 5, 2015

I actually find that last argument (strict mode) compelling and am closing this PR.

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