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.

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

http: refactor to make servername option normalization testable #38733

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 19, 2021

@github-actions github-actions bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels May 19, 2021
Comment on lines +341 to +349
function normalizeServerName(options, req) {
if (!options.servername && options.servername !== '')
options.servername = calculateServerName(options, req);
}

Copy link
Member

Choose a reason for hiding this comment

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

Should we avoid mutating an argument in a function?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be good to refactor that away. But can happen in a follow up PR.

Comment on lines +341 to +349
function normalizeServerName(options, req) {
if (!options.servername && options.servername !== '')
options.servername = calculateServerName(options, req);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be good to refactor that away. But can happen in a follow up PR.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit b866e38 into nodejs:main Oct 6, 2023
21 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Oct 6, 2023

Landed in b866e38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants