Skip to content

src: remove regex usage for env file parsing - #52406

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
IlyasShabi:dotenv-refacto
Apr 17, 2024
Merged

src: remove regex usage for env file parsing#52406
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
IlyasShabi:dotenv-refacto

Conversation

@IlyasShabi

@IlyasShabiIlyasShabi commented Apr 7, 2024

Copy link
Copy Markdown
Member

@nodejs-github-botnodejs-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. labels Apr 7, 2024
@IlyasShabi
IlyasShabi marked this pull request as ready for review April 7, 2024 15:35

@climba03003climba03003 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add multiple continues comments in valid.env to ensure it works?

@IlyasShabi

Copy link
Copy Markdown
MemberAuthor

Can you add multiple continues comments in valid.env to ensure it works?

@climba03003 Could you provide me with an example and the expected result?

@climba03003

climba03003 commented Apr 7, 2024

Copy link
Copy Markdown
Contributor
// .valid.env
#COMMENTED_ENV=shouldnotshown
#COMMENTED_ENV_FOLLOW_COMMENTED_ENV=shouldnotshown
#COMMENTED_ENV_FOLLOW_COMMENTED_ENV_2=shouldnotshown// test// Commented environment should be undefinedassert.strictEqual(process.env.COMMENTED_ENV,undefined);assert.strictEqual(process.env.COMMENTED_ENV_FOLLOW_COMMENTED_ENV,undefined);assert.strictEqual(process.env.COMMENTED_ENV_FOLLOW_COMMENTED_ENV_2,undefined);

@VoltrexKeyva

Copy link
Copy Markdown
Contributor

You should use the snake_case naming convention for local variables and parameters as stated here in the C++ style guide.

Comment threadtest/parallel/test-dotenv.js Outdated
Comment threadsrc/node_dotenv.cc Outdated
Comment threadtest/parallel/test-dotenv.js Outdated

@anonriganonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small changes but overall I think this is ready to land. Thank you.

Comment threadsrc/node_dotenv.cc Outdated
Comment threadsrc/node_dotenv.cc Outdated
Comment threadsrc/node_dotenv.cc Outdated
Comment threadsrc/node_dotenv.cc Outdated
Comment threadsrc/node_dotenv.cc Outdated
@anonriganonrig 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 Apr 11, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 11, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@IlyasShabi

Copy link
Copy Markdown
MemberAuthor

Flaky tests

@targos

Copy link
Copy Markdown
Member

No. Some dotenv tests are broken on Windows.

See https://ci.nodejs.org/job/node-test-binary-windows-js-suites/27085/RUN_SUBSET=1,nodes=win2016-COMPILED_BY-vs2022-x86/console

(Search for "not ok" in the output)

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2024
Comment threadsrc/node_dotenv.cc Outdated
@anonriganonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 16, 2024
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 16, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/52406
✔ Done loading data for nodejs/node/pull/52406
----------------------------------- PR info ------------------------------------
Title src: remove regex usage for env file parsing (#52406)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch IlyasShabi:dotenv-refacto -> nodejs:main
Labels c++, author ready, needs-ci, commit-queue-squash
Commits 2
- src: remove regex usage for env file parsing
- src: handle empty value without newline at EOF
Committers 1
- Ilyas Shabi PR-URL: https://github.com/nodejs/node/pull/52406
Fixes: https://github.com/nodejs/node/issues/52248
Reviewed-By: Yagiz Nizipli ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52406
Fixes: https://github.com/nodejs/node/issues/52248
Reviewed-By: Yagiz Nizipli --------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - src: handle empty value without newline at EOF
ℹ This PR was created on Sun, 07 Apr 2024 15:34:18 GMT
✔ Approvals: 1
✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/52406#pullrequestreview-1995581300
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-04-16T01:40:19Z: https://ci.nodejs.org/job/node-test-pull-request/58417/
- Querying data for job/node-test-pull-request/58417/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8699372234

Comment threadsrc/node_dotenv.cc Outdated
@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@targostargos added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 17, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 3f88e14 into nodejs:mainApr 17, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 3f88e14

@kibertoad

Copy link
Copy Markdown
Contributor

@IlyasShabi thank you so much! do you plan to backport to 20.x too?

@kibertoad

kibertoad commented Apr 27, 2024

Copy link
Copy Markdown
Contributor

@RafaelGSS I don't see this PR in the changelog for 22.0.0, was it somehow excluded for whatever reason? It doesn't seem to be fixed in current V22 either.

@anonrig

Copy link
Copy Markdown
Member

cc @nodejs/releasers

aduh95 pushed a commit that referenced this pull request Apr 29, 2024
PR-URL: #52406Fixes: #52248
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@targostargos mentioned this pull request Apr 30, 2024
8 tasks
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #52406Fixes: #52248
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@marco-ippolitomarco-ippolito mentioned this pull request May 2, 2024
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #52406Fixes: #52248
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@anonriganonrig mentioned this pull request Aug 1, 2024
@atilkan

Copy link
Copy Markdown

Is it possible that with this change inline comments are gone?

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

Labels

author readyPRs 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++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@IlyasShabi@climba03003@VoltrexKeyva@nodejs-github-bot@targos@kibertoad@anonrig@atilkan@aduh95@ilyasShabiCS