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: remove util from test-repl-setprompt #3338

Closed
wants to merge 1 commit into from
Closed

test: remove util from test-repl-setprompt #3338

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

Util is being used only for string templating. Inspired by #3324, this commit removes util from the test, and instead uses back ticks to do the templating. The commit also replaces the var declaration with const, and replaces the leading comma style used for variable declaration to match the style found in other tests.

Util is being used only for string templating. Inspired by #3324, this commit removes util from the test, and instead uses back ticks to do the templating. The commit also replaces the var declaration with const, and replaces the leading comma style used for variable declaration to match the style found in other tests.
@MylesBorins
Copy link
Contributor Author

/cc @jasnell @Trott

@Trott
Copy link
Member

Trott commented Oct 13, 2015

LGTM if CI is OK.

CI: https://ci.nodejs.org/job/node-test-commit/813/

@jasnell
Copy link
Member

jasnell commented Oct 13, 2015

Good to see this kind of cleanup happening. LGTM so long as CI is green.

@Trott Trott added the test Issues and PRs related to the tests. label Oct 13, 2015
@rvagg
Copy link
Member

rvagg commented Oct 13, 2015

these probably would have been easier to manage and review in a single PR and single commit, as it is we're going to end up with a fair bit of noise for what's really just a single change across multiple files

@Fishrock123
Copy link
Contributor

Yes, @thealphanerd for changes like this where it is all very similar it would be preferred to keep it to a single PR & commit. :)

@MylesBorins
Copy link
Contributor Author

@rvagg / @Fishrock123 thanks for the heads up.

Will keep similar cleanup in a single PR in the future.

@Trott
Copy link
Member

Trott commented Oct 14, 2015

@rvagg @Fishrock123 I was about to land this and the two related PRs, but before I do (or someone else does because I'm about to go to bed): Should I squash the three of them into a single commit? The only issue I can think of with doing that is that there will be three PR-URL metadata lines in the commit.

¯\_(ツ)_/¯

@MylesBorins
Copy link
Contributor Author

@Trott feel free to take liberty in changing the commit title to better reflect all three living in one place

@Trott
Copy link
Member

Trott commented Oct 14, 2015

(Alternative: Close these three PRs, ask @thealphanerd to cherry-pick the commits from the three PRs onto a clean branch, optionally squash, open a new PR. Only advantage I can think of to that is if it's important that there be only one PR-URL line per commit.)

@MylesBorins
Copy link
Contributor Author

I can do that before the am

@MylesBorins
Copy link
Contributor Author

done :D

@MylesBorins MylesBorins deleted the removeUtil-repl-setprompt branch October 14, 2015 07:20
@MylesBorins
Copy link
Contributor Author

this should go to LTS

/cc @jasnell

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants