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.

Sign up for GitHub

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

Jump to bottom

Backport of #5778 #5960

Closed
wants to merge 1 commit into from
Closed

Backport of #5778 #5960

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 30, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Please provide a description of the change here.

Convert string concatenation to template literals. Enforce with lint
rule.

@Trott Trott added the v4.x label Mar 30, 2016
@Trott Trott mentioned this pull request Mar 30, 2016
Closed
4 tasks
@MylesBorins
Copy link
Contributor

@Trott can you keep the original review data please 😄

@Trott
Copy link
Member Author

Trott commented Mar 30, 2016

Original review metadata restored. Guess you'll want to update the PR number, eh? I left it as the old one. Wasn't sure what your procedure is. Just pointing out in case I guessed wrong.

@MylesBorins
Copy link
Contributor

I usually leave the old one so that everything sync's up with branch diff

@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 30, 2016
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

@Trott .. looks like your branch needs to be rebased to clear out the extraneous commits here.

@Trott
Copy link
Member Author

Trott commented Apr 1, 2016

@jasnell rebased and force pushed. There still seems to be one extraneous commit. ???

@MylesBorins
Copy link
Contributor

@Trott can you rebase out e42bd4a

@Trott
Copy link
Member Author

Trott commented Apr 1, 2016

@thealphanerd OK, done!

@MylesBorins
Copy link
Contributor

@Trott this commit is blowing up the linter. Look like there is still a whole bunch of non template literal string concatenation happening in v4.x... 236 errors in total.

This amount of churn does make me a bit uneasy

@jasnell
Copy link
Member

jasnell commented Apr 3, 2016

Yeah, I'd say let's hold off on this given that high number of failures.

@Trott
Copy link
Member Author

Trott commented Apr 3, 2016

Heh, that's because I modified the wrong .eslintrc. It was supposed to go in src and not lib but I put it in lib and not src. Whoops. Sit tight...

@Trott
src,tools: use template literals
62cb6ca 
Convert string concatenation to template literals. Enforce with lint
rule.

PR-URL: nodejs#5778
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@Trott
Copy link
Member Author

Trott commented Apr 3, 2016

OK! Linter passes now. Sheesh. Sorry about that.

@MylesBorins
Copy link
Contributor

landed in 4f683ab

@MylesBorins MylesBorins closed this Apr 4, 2016
@MylesBorins MylesBorins removed their assignment Dec 27, 2016
@Trott Trott deleted the cleats branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @MylesBorins @jasnell @mscdex @Fishrock123