Skip to content

src: update contextify to use DictionaryTemplate #60059

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
Merged
merged 1 commit into from
Oct 6, 2025
Merged

src: update contextify to use DictionaryTemplate #60059

merged 1 commit into from
Oct 6, 2025
+63 −43

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 28, 2025

More use of DictionaryTemplate

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Sep 28, 2025
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 29, 2025
BridgeAR

This comment was marked as resolved.

Sign in to view

legendecas
legendecas reviewed Sep 29, 2025
View reviewed changes
@jasnell jasnell force-pushed the jasnell/contextify-dictionarytemplate branch from 67a08e7 to 956b245 Compare October 4, 2025 12:53
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@codecov Codecov

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@addaleax

This comment was marked as resolved.

Sign in to view
@jasnell jasnell force-pushed the jasnell/contextify-dictionarytemplate branch from 956b245 to fa77052 Compare October 5, 2025 12:47
@jasnell jasnell requested a review from BridgeAR October 5, 2025 12:48
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/69570/

@jasnell jasnell requested a review from addaleax October 5, 2025 12:48
legendecas
legendecas requested changes Oct 5, 2025
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

It is not convincing that a 20-line if-condition is more readable than simple early returns. I don't believe this beneficial change has to be bundled with a subjective style change.

@jasnell jasnell force-pushed the jasnell/contextify-dictionarytemplate branch from fa77052 to 23601da Compare October 5, 2025 22:45
@jasnell jasnell force-pushed the jasnell/contextify-dictionarytemplate branch from 23601da to b53b4ec Compare October 5, 2025 22:55
@jasnell jasnell requested a review from legendecas October 5, 2025 22:55
@jasnell
Copy link
Member Author

jasnell commented Oct 5, 2025

Blocking an otherwise fully correct PR for a purely subjective style change seems more counterproductive, especially when the style change is consistent with lots of other places throughout the codebase, but ok. I switched it back to the multiple separate if statements.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/69576/

legendecas
legendecas approved these changes Oct 6, 2025
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thanks!

@addaleax addaleax added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 6, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2025
@nodejs-github-bot nodejs-github-bot merged commit 4a7fbb6 into nodejs:main Oct 6, 2025
60 of 61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4a7fbb6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@tniessen tniessen tniessen approved these changes

@legendecas legendecas legendecas approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasnell @nodejs-github-bot @addaleax @tniessen @legendecas @BridgeAR