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

src: remove erroneous dot #12626

Closed
wants to merge 1 commit into from
Closed

src: remove erroneous dot #12626

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

This got accidentally added when landing original commit

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

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

@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Apr 24, 2017

LGTM

@MylesBorins
Copy link
Contributor Author

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

@seishun
Copy link
Contributor

seishun commented Apr 24, 2017

How about "extraneous" instead of "erroneous"?

@jasnell jasnell mentioned this pull request Apr 24, 2017
Closed
2 tasks
@MylesBorins
Copy link
Contributor Author

https://ci.nodejs.org/job/node-test-linter/8519/

@MylesBorins
src: remove extraneous dot
9357772 
This got accidentally added when landing original commit
@MylesBorins
Copy link
Contributor Author

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

@MylesBorins
Copy link
Contributor Author

landed in 9cc39ff

@MylesBorins
Copy link
Contributor Author

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
@MylesBorins
src: remove extraneous dot
9cc39ff 
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 •
edited
Loading

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
Contributor Author

@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

jasnell commented Apr 24, 2017

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 commented Apr 24, 2017

@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

refack commented Apr 24, 2017

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

jasnell commented Apr 24, 2017

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

@refack
Copy link
Contributor

refack commented Apr 25, 2017

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
@MylesBorins @evanlucas
src: remove extraneous dot
f05eb4b 
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 evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@MylesBorins @evanlucas
src: remove extraneous dot
0c7f663 
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
@MylesBorins @evanlucas
src: remove extraneous dot
cfd9144 
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]>
@MylesBorins MylesBorins deleted the fix-dot branch November 14, 2017 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@seishun seishun seishun approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MylesBorins @jasnell @mscdex @seishun @refack @nodejs-github-bot