End-of-line comments, quoted whitespace, and other combination edge cases #158
Conversation
- EOL comment tests initially fail since they aren't implemented. - Unquoted-whitespace-trim test is fine. - Quoted-whitespace-notrim test fails.
It mostly works, except it fails the edge case when hashes fall within quotes (in which case it should be treated as part of the value and not a comment).
The tests successfully pass.
|
This would be better as two separate pull requests– one to address inline comments and another to address trimming whitespace in quotes. In regards to inline comments, I think it adds more complexity than usefulness. No one has presented any use cases. Why do you need a comment about a key value pair that can't go on the line above? In regards to trimming whitespace, is this actually a problem? If lots of people are having their values incorrectly parsed, let's fix it, but if no one has noticed this for years, let's leave it alone. |
|
Firstly, apologies for conflating the 2 issues into the same PR. The whitespace change was a secondary issue encountered while trying to implement EOL comments, and I bundled it because their code lines weren't fully independent. In an event where you'd like to go ahead with 1 change but not the other, I'll be happy to issue a new PR implementing only the approved change. I've replied about inline comments in its original issue, so maybe we can continue with whitespace here. On my end, my reasoning for proposing to change the whitespace treatment was because I was assuming that 1) more specification, rather than less, about how the module's features interact (e.g. quotes vs. whitespace handling) is generally better, and 2) in that case, the current specification for how whitespace gets trimmed even when enclosed within quotes does seem a bit odd. 1 case I can come up with (post-hoc) for the change is the event whereby whitespace is meant to be the actual value being configured, where given the current implementation they would end up get trimmed to an empty string: INDENT_PADDING=" " # 2 spaces
DELIMITER=" " # tab characterBut yes, I admit that this situation may be unlikely, to the point where no one using the module in its history has encountered it yet. |
|
@sneakertack I would have to agree with @maxbeatty on the "white space in quotes" issue. I don't think we should bundle that into this PR. I think it added some extra complexity that is not needed to achieve the EOL comments parsing. I would say try not to open a new PR, try to revert those lines. It is always good to keep a accurate history on a PR. |
Old behaviour is that whitespace gets trimmed, whether quoted or unquoted.
|
Okay, I've reverted the quoted-whitespace change and updated the tests/readme accordingly. It doesn't necessarily decrease the code line changes in the |
2 similar comments
|
sorry, this really breaks people's expectations of how dotenv should work out of the box (like any password with a # is going to get truncated if it's not quoted). low value feature + additional code complexity + breaking expectations ≠ good |
This PR implements the following changes:
The main motivation is (1), as I too would like to be able to comment at the end of lines; but I suspect that the addition of a PEG.js parser might be a bit too heavy-handed (adds >600 new SLOC) when this library should be lightweight/zero-dependency.
It has 4 commits:
parsefunction (modify the regex + removetrim()). It almost works, however, 1 test doesn't pass (it fails in the tricky case of combining quotes and the comment hash character). This hopefully justifies why I had to resort to making a bigger change next.parsefunction to better handle quotes + hashes. The prior failing test now passes.All tests are passing in my local environment. I've also run it against the linter. Discussion and requests for amendment are welcome.