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

test: minor replace var by const #8621

Closed
wants to merge 1 commit into from

Conversation

scips
Copy link
Contributor

@scips scips commented Sep 17, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. Could you update the commit message to reference the specific test?

@scips scips force-pushed the improve-test-tls-zero-clear-in branch from 0336cb9 to 308d155 Compare September 17, 2016 13:00
@scips
Copy link
Contributor Author

scips commented Sep 17, 2016

@cjihrig changed to test: replace var by const test-tls-zero-clear-in

@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2016

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Sep 17, 2016
@imyller
Copy link
Member

imyller commented Sep 17, 2016

CI failures with arm-fanned are unrelated.

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@benjamingr
Copy link
Member

I thought we were against these sort of changes and that they were churn unless they introduce changes that are not changes from var to const.

@imyller
Copy link
Member

imyller commented Sep 19, 2016

@benjamingr This is related to nodejs/code-and-learn#56 (comment)

I'll start the process of landing this.

@imyller imyller self-assigned this Sep 19, 2016
imyller pushed a commit that referenced this pull request Sep 20, 2016
PR-URL: #8621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller
Copy link
Member

imyller commented Sep 20, 2016

landed in 5f4bc59

Thank you for your contribution, @scips

@imyller imyller closed this Sep 20, 2016
@imyller imyller removed their assignment Sep 20, 2016
@addaleax
Copy link
Member

I thought we were against these sort of changes and that they were churn unless they introduce changes that are not changes from var to const.

Although I don’t have any strong feelings about this: Yes, that was more or less my impression, too. I think the problematic effects are definitely smaller for test files, and overall I’m sorry for not being prepared for the number of attendees and falling back to a more ad-hoc way of distributing first contributions that may have led to some unclarity as to what would be considered a proper change and what not.

I do think the value of having gained quite a few people who are now familiar with the contribution process is non-negligible, though.

@benjamingr This is related to nodejs/code-and-learn#56 (comment)

I’m not sure if @benjamingr’s comment was meant as an objection, but if so, maybe landing this was a bit premature?

@benjamingr
Copy link
Member

@addaleax @imyller it was not an objection, I was just not playing really close attention and was surprised. If this is meant as a way to get people more acquainted with our code base and getting contributors I'm definitely in favor of it 👍

@imyller
Copy link
Member

imyller commented Sep 20, 2016

I’m not sure if @benjamingr’s comment was meant as an objection, but if so, maybe landing this was a bit premature?

Good point @addaleax.

I personally didn't read his comment as an objection to this specific PR (in wider context of other code+learn PRs), but definitely try be more sensitive when evaluating feedbacks in other PRs. Thank you.

I've taken up the task of landing these code+learn session contributions and I've tried to be bit more forgiving to new contributors about minor details (commit title wording) and fix those in the landing process. Hopefully thats ok?

@addaleax
Copy link
Member

I've taken up the task of landing these code+learn session contributions and I've tried to be bit more forgiving to new contributors about minor details (commit title wording) and fix those in the landing process. Hopefully thats ok?

Imho, that is not only ok, that is awesome. 👍

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
PR-URL: #8621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants