src: make parsing compatible with motdotla/dotenv package #54215
Conversation
|
Review requested:
|
nodejs-github-bot
added
build
|
Hmm, no idea which subsystem to use here 🤔. I see that previous commits in these files were marked as |
src/node_dotenv.cc
Outdated
| auto first = input.front(); | ||
| if ((first == '\'' || first == '"' || first == '`') && | ||
| input.back() == first) { | ||
| input = input.substr(1, input.size() - 2); |
There was a problem hiding this comment.
A more readable way:
input.remove_prefix(1);
input.remove_suffix(2);
|
|
||
| void Dotenv::ParseContent(const std::string_view input) { | ||
| std::string lines(input); | ||
| std::string_view parse_key(std::string_view key) { |
There was a problem hiding this comment.
Can you add a comment to what this function does?
src/node_dotenv.cc
Outdated
| // Expand \n to newline in double-quote strings | ||
| size_t pos = 0; | ||
| auto expanded = std::string(trimmed); | ||
| while ((pos = expanded.find("\\n", pos)) != std::string_view::npos) { |
There was a problem hiding this comment.
expanded is a string now. std::string::npos is the correct return value here.
src/node_dotenv.cc
Outdated
| if (value.empty()) return ""; | ||
|
|
||
| auto trimmed = trim_quotes(value); | ||
| if (value.front() == '\"' && value.back() == '\"') { |
There was a problem hiding this comment.
Can you add a documentation to here?
Returning the else statement early will make this function much more readable.
| std::string::size_type start = 0; | ||
| std::string::size_type end = 0; | ||
|
|
||
| for (std::string::size_type i = 0; i < input.size(); i++) { |
There was a problem hiding this comment.
You removed all of the comments in this function, and it is a lot less readable right now. I can't review it with the current state.
There was a problem hiding this comment.
This is basically a new function. Old one was doing a lot of back & forth searching for chars and newlines in string and it was hard for me to fit in a fix. This goes through the content of file only once and parses it char by char.
src/node_dotenv.cc
Outdated
| } | ||
| } | ||
|
|
||
| std::optional<std::string> Dotenv::GetValue(const std::string_view key) const { |
There was a problem hiding this comment.
This can return std::string_view since the underlying store will not be disposed.
| std::optional<std::string> Dotenv::GetValue(const std::string_view key) const { | |
| std::optional<std::string_view> Dotenv::GetValue(const std::string_view key) const { |
src/node_dotenv.cc
Outdated
| auto match = store_.find(key.data()); | ||
|
|
||
| if (match != store_.end()) { | ||
| return std::optional<std::string>{match->second}; |
There was a problem hiding this comment.
| return std::optional<std::string>{match->second}; | |
| return match->second; |
test/parallel/test-dotenv.js
Outdated
|
|
||
| // These are no longer true, parser is now more strict when it comes to balancing double quotes | ||
| // assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"'); | ||
| // assert.strictEqual(process.env.MULTI_NOT_VALID, 'THIS'); | ||
|
|
There was a problem hiding this comment.
| // These are no longer true, parser is now more strict when it comes to balancing double quotes | |
| // assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"'); | |
| // assert.strictEqual(process.env.MULTI_NOT_VALID, 'THIS'); |
marekpiechut
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54215 +/- ##
=======================================
Coverage 87.09% 87.09%
=======================================
Files 647 647
Lines 181845 181900 +55
Branches 34913 34924 +11
=======================================
+ Hits 158373 158426 +53
+ Misses 16751 16749 -2
- Partials 6721 6725 +4
|
|
Hey @anonrig did you have a chance to take a second look? I've resolved all small things and only thing left is the parsing function that I have rewritten. If you think that the change is too risky I can revert it, keep the tests and try to fit in the fix in current implementation. |
|
This appears to be waiting for feedback from @nodejs/config. |
5 participants
Fixes: #54134