Support envfile task level and global option #29
Conversation
poethepoet/helpers.py
Outdated
|
|
||
| else: | ||
| # We found one or more backslashes | ||
| num_backslashes = len(match_terminator) |
There was a problem hiding this comment.
I see what you're doing. But won't the regex
((?:".*?(?<!\\)(\\\\)*?")|(?:'.*?(?<!\\)(\\\\)*?'))Be sufficient?
Explanation
- 2 groups: one for handling
"and one for handling'. - The middle part (
.*?) should be self-explanatory - The last part (
(?<!\\)(\\\\)*?) will match an even number of\that are not escaped.
There was a problem hiding this comment.
Hmm, it seemed simpler this way, but that's worth exploring. Note that backslashes don't work as escapes within single quotes, and we need remove active quotes from the result but preserve escaped backslashes as in lines 84 & 120.
| """, | ||
| {"FOO": "BAR", "BAR": "FOO"}, | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Can we add a test for space before the FOO, too? And, instead of strictly complying, why not allow spaces between the =?
There was a problem hiding this comment.
Look again, preceding space is present in all the tests 💯 Maybe there should be a test without preceding space...
I considered allowing spaces around the = though felt that sticking to how bash does it was a safer bet design wise. I figured it would be least surprising if the file is loaded exactly the same way as it would be via the source command. Maybe this is worth revisiting, though I would need evidence to motivate a change.
|
@ThatXliner Thanks for taking a look.
I tried it with a few simple scenarios, I intend it test it more and also add a few scenarios to the feature test suite before merging and making a pre-release, when I get another block of time to work on it. The "UI" line changes are all about being able to print a warning at context.py:90 when the UI is in verbose mode. |
nat-n
force-pushed
the
feature/envfile
branch
2 times, most recently
from
1671091
to
101eb9f
Compare
|
The current envfile parsing is very fault tolerant, which is good, but doesn't emit warnings for syntax errors. With this envfile: TEST=yeetus # Epic
e = A
A = b
a = b
a = cOnly |
Good catch, regexes are tricky beasts. I added some tests with bad input to give more confidence. |
nat-n
force-pushed
the
feature/envfile
branch
3 times, most recently
from
a170ce7
to
7b0f6f7
Compare
|
Good, it's erroring now. Except the error message is very cryptic... You might wanna use something like Lark which has very epic error message support. (Lark can generate a standalone so you don't need to worry about an extra dependency) |
nat-n
force-pushed
the
feature/envfile
branch
3 times, most recently
from
cd0d9fe
to
26b34e0
Compare
- Includes original envfile parser that closely follows bash syntax
Good point. I tweaked the error handling so it's a bit more specific and helpful I think. I think it's good to go now, but I'll leave this open a short while longer in case you'd like to give more feedback before I merge and create another pre-release. |
|
Looks good. It would be nice to have suggestions but at least the errors are correct. |
Adds support for a new
envfileoption at global or task level. #22Includes an original lightweight parser for envfiles closely following bash syntax.