GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: remove `node_bob.h` and `node_bob-inl.h` by RaisinTen · Pull Request #44607 · nodejs/node

Conversation

@RaisinTen

@RaisinTen RaisinTen commented Sep 12, 2022

Copy link
Copy Markdown
Member

These were added in #32379 and were supposed to get removed in #37067.

Signed-off-by: Darshan Sen [email protected]

cc @jasnell

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax and bricss reacted with heart emoji
All reactions
  • ❤️ 2 reactions
@RaisinTen
src: remove node_bob.h and node_bob-inl.h
e514969 
These were added in nodejs#32379 and were
supposed to get removed in nodejs#37067.

Signed-off-by: Darshan Sen <[email protected]>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 12, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
bnoordhuis
bnoordhuis approved these changes Sep 12, 2022
View reviewed changes
@RaisinTen RaisinTen added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 12, 2022
@tniessen

tniessen commented Sep 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Just want to make sure this is not going to make James' work more difficult in #44325... cc @jasnell. (There's also a few leftovers in the crypto code base that I have not removed yet because they might still be needed for the upcoming QUIC implementation.)

Looks like these files are required for that PR and would just be re-added. Maybe we should wait with removing more prerequisites of #44325.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Sep 13, 2022
CI Reliability 2022-09-13 nodejs/reliability#376
Open
30 tasks
JungMinu
JungMinu approved these changes Sep 13, 2022
View reviewed changes
addaleax
addaleax approved these changes Sep 13, 2022
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Sep 13, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/46564/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Sep 14, 2022
CI Reliability 2022-09-14 nodejs/reliability#378
Open
CI Reliability 2022-09-15 nodejs/reliability#379
Open
tniessen
tniessen requested changes Sep 15, 2022
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just making my concern explicit. I don't think we should be removing this while the new QUIC PR depends on these files. It's already a ton of work for James and re-adding files with no changes just seems to add to it.

That's just my take though and I'm only requesting changes so that this point is not missed. If someone believes this should land regardless, I'll gladly dismiss my review.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
This was referenced Sep 16, 2022
CI Reliability 2022-09-16 nodejs/reliability#380
Open
CI Reliability 2022-09-17 nodejs/reliability#381
Open
@RaisinTen RaisinTen removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 18, 2022
@RaisinTen

RaisinTen commented Sep 18, 2022

Copy link
Copy Markdown
Member Author

That was an oversight on my part. I have no intentions of making James' work more difficult, closing. :)
tniessen reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen RaisinTen closed this Sep 18, 2022
@RaisinTen
RaisinTen deleted the src/remove-node_bob branch September 18, 2022 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen requested changes
@bnoordhuis bnoordhuis bnoordhuis approved these changes
@addaleax addaleax addaleax approved these changes
@JungMinu JungMinu JungMinu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@RaisinTen @nodejs-github-bot @tniessen @bnoordhuis @addaleax @JungMinu
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.