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

src: fix dotenv parsing commented env #52363

Closed
wants to merge 3 commits into from

Conversation

climba03003
Copy link
Contributor

@climba03003 climba03003 commented Apr 4, 2024

Fixes #52084
Fixes #52362

@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. labels Apr 4, 2024
"\\s*(?:export\\s+)?([\\w.-]+)(?:\\s*=\\s*?|:\\s+?)(\\s*'(?:\\\\'|[^']"
")*'|\\s*\"(?:\\\\\"|[^\"])*\"|\\s*`(?:\\\\`|[^`])*`|[^#\r\n]+)?\\s*(?"
":#.*)?"); // NOLINT(whitespace/line_length)
"(?:^|^)\\s*(?:export\\s+)?([\\w.-]+)(?:\\s*=\\s*?|:\\s+?)(\\s*'(?"
Copy link
Contributor Author

@climba03003 climba03003 Apr 4, 2024

Choose a reason for hiding this comment

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

The previous implementation is copied from dotenv but the regexp is missing the beginning part (?:^|^).

So, it cannot determine the commented env properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it is removed because std::regex::multiline is not supported.
The current workflow using C++11 only and that required C++17

Copy link

Choose a reason for hiding this comment

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

I'm not sure that (?:^|^) part is makes much sense, and can be simplified just to (?:^)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After testing, it can be simplified to (?:^).
Due the problem of std::regex::multiline not supported in some environment.
I believe the option of just changing the regex is a no-go.

@climba03003
Copy link
Contributor Author

climba03003 commented Apr 4, 2024

After searching around #52120 also solve the issue.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

We should move away from regexp implementation due to Windows limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
4 participants