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 repl reset event #16836

Closed

Conversation

AdriVanHoudt
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017
@AdriVanHoudt
Copy link
Contributor Author

I'm not super sure about the wording so any feedback welcome

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Nov 6, 2017
@gireeshpunathil
Copy link
Member

@@ -61,7 +64,8 @@ function testResetGlobal() {
assert.strictEqual(
context.foo,
42,
'"foo" property is missing from REPL using global as context'
'"foo" property is different from REPL using global as context.' +
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Probably should be a space at the end of the string on this line or else a space at the beginning of the string on the next line? Same for two other instances above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, tell me what'd prefer :P

Copy link
Member

Choose a reason for hiding this comment

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

@AdriVanHoudt I think we prefer @Trott's suggested style for most of the codebase :)

Copy link
Member

Choose a reason for hiding this comment

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

@AdriVanHoudt Space at the end is more typical throughout the code base as far as I've noticed, so go with that I guess? But either would be fine.

@AdriVanHoudt
Copy link
Contributor Author

@Trott I added a space at the end of every line, let me know if I need to squash this

@Trott
Copy link
Member

Trott commented Dec 4, 2017

@AdriVanHoudt
Copy link
Contributor Author

The space made line 47 to long 🤦‍♂️

@Trott
Copy link
Member

Trott commented Dec 4, 2017

The space made line 47 to long 🤦‍♂️

😃 If you could wrap and fix, that would be great. (And if not, someone else like me can come along and do it when we're ready to land. But saving that person a few keystrokes and some time would be great.)

@AdriVanHoudt
Copy link
Contributor Author

Do you mean splitting the string in 3 parts instead of the current 2? I feel like eslint should be able to fix this 🙃

@Trott
Copy link
Member

Trott commented Dec 5, 2017

Do you mean splitting the string in 3 parts instead of the current 2?

Easiest fix is probably to remove the comma after context on line 47.

I feel like eslint should be able to fix this 🙃

ESLint docs indicate max-len is not a rule that can be auto-fixed.

@AdriVanHoudt
Copy link
Contributor Author

ESLint docs indicate max-len is not a rule that can be auto-fixed.

Ok that rule does more than I expected, shortsighted comment from me sorry

I updated the line, so let's hope it is good now

@Trott
Copy link
Member

Trott commented Dec 6, 2017

apapirovski pushed a commit that referenced this pull request Dec 9, 2017
PR-URL: #16836
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@apapirovski
Copy link
Member

Landed in bbcbcb8

Thank you for the contribution @AdriVanHoudt!

@apapirovski apapirovski closed this Dec 9, 2017
@AdriVanHoudt AdriVanHoudt deleted the test-repl-reset-event branch December 11, 2017 07:14
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16836
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16836
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #16836
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #16836
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #16836
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
@MylesBorins MylesBorins mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants