Skip to content

Conversation

@MylesBorins
Copy link
Contributor

This got accidentally added when landing original commit

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 24, 2017
@MylesBorinsMylesBorins mentioned this pull request Apr 24, 2017
2 tasks
@MylesBorins
Copy link
ContributorAuthor

@nodejs/collaborators can we get some v quick LGTM's on this so we can unbreak master

@jasnell
Copy link
Member

fyi.. there's a linting error in there also #12625

@mscdex
Copy link
Contributor

LGTM

@MylesBorins
Copy link
ContributorAuthor

@jasnell I'll run CI but I believe I got the linting error in here. should trump #12625

@seishun
Copy link
Contributor

How about "extraneous" instead of "erroneous"?

@jasnelljasnell mentioned this pull request Apr 24, 2017
2 tasks
@MylesBorins
Copy link
ContributorAuthor

This got accidentally added when landing original commit
@MylesBorins
Copy link
ContributorAuthor

Linter is green, updated commit message as per @seishun's request

@MylesBorins
Copy link
ContributorAuthor

landed in 9cc39ff

@MylesBorins
Copy link
ContributorAuthor

I'm going to opt to manually fix in the backports rather than float both patches

MylesBorins added a commit that referenced this pull request Apr 24, 2017
This got accidentally added when landing original commit PR-URL: #12626 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nikolai Vavilov <[email protected]>
@refack
Copy link
Contributor

refack commented Apr 24, 2017

I'm going to opt to manually fix in the backports rather than float both patches

🏁(me raising my "nudnik" flag)
So why did this go into master in the first place, and not directly into v6.x-staging?
Master was green before #12540. just for explicitly?

@MylesBorins
Copy link
ContributorAuthor

@refack obviously there was something going on without this header. Makes sense to land to master and then backport. The failing was due to me landing it improperly... which occasionally happens. We fixed it in less than an hour, which is pretty reasonable imho.

@jasnell
Copy link
Member

We always land changes in master first unless the code being changed is a specific bug in code that no longer exists in master.

@refack
Copy link
Contributor

@refack obviously there was something going on without this header. Makes sense to land to master and then backport. The failing was due to me landing it improperly... which occasionally happens. We fixed it in less than an hour, which is pretty reasonable imho.

@MylesBorins
No critique on the bad land, and fix. Great work!
I'm asking about #12540. obviously there was something going on without this header is what I wanted to know. Thank you!

We always land changes in master first unless the code being changed is a specific bug in code that no longer exists in master.

@jasnell IMHO This was a borderline case, but I defer to @MylesBorins's reasoning.

@refack
Copy link
Contributor

Friends, do you think this should be documented?
is you add a new macro or use a new external function, make sure you include the header file explicitly in the file you are editing. This makes sure your change is independent of previous changes, and makes it easier to backport

@jasnell
Copy link
Member

@refack ... a separate discussion issue would be better for that as it's not relevant to this specific PR

@refack
Copy link
Contributor

Will do.
Just had to do a brain dump, and wanted to feel the wind's direction...

evanlucas pushed a commit that referenced this pull request Apr 25, 2017
This got accidentally added when landing original commit PR-URL: #12626 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nikolai Vavilov <[email protected]>
@evanlucasevanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
This got accidentally added when landing original commit PR-URL: #12626 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nikolai Vavilov <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
This got accidentally added when landing original commit PR-URL: #12626 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Nikolai Vavilov <[email protected]>
@MylesBorinsMylesBorins deleted the fix-dot branch November 14, 2017 17:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@MylesBorins@jasnell@mscdex@seishun@refack@nodejs-github-bot