dub.project: Improve variable expansion logic#1641
Conversation
Replace the previous buggy logic with a simpler and more efficient implementation.
CyberShadow
force-pushed
the
pull-20190120-125417
branch
from
6b8bd6b to
e06e572
Compare
|
Thanks for your pull request, @CyberShadow! |
CyberShadow
changed the title
| assertThrown(expandVars!expandVar("${X")); | ||
|
|
||
| // https://github.com/dlang/dmd/pull/9275 | ||
| assert(expandVars!expandVar("$${DUB_EXE:-dub}") == "${DUB_EXE:-dub}"); |
There was a problem hiding this comment.
Should we test for other Bash variable syntaxes: https://www.tldp.org/LDP/abs/html/parameter-substitution.html ?
There was a problem hiding this comment.
Well, the point of this test is to explicitly check that Dub doesn't do anything crazy here, and it ignores anything following $$ and passes it through verbatim, whatever the syntax might be. The previous implementation expected very specific syntax to be used even when escaping Dub processing, which was presumptuous and led to wrong behavior. This test (and any other tests we might add in the same vein) are supplemental / illustrative to the behavior tests above, and don't actually add to the test coverage. An implementation of expandVars which somehow passes all of these tests but would behave incorrectly with other kinds of Bash variable substitution syntax would be very weird indeed.
There was a problem hiding this comment.
Ok, good enough for me.
|
LGTM. |
@CyberShadow Yes, it is working as intended - it's just that we have a different definition of what's intended. I asked for input on this matter on the newsgroup, but apparently no one cared enough to respond at the time, so we went with the current behavior. |
|
@ZombineDev Sorry, what do you mean? Of the unit tests I added, how would you consider an implementation that failed them as "working as intended"? |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
The new implementation is so much more readable 👍
|
I'm going to merge this now as it still passes all the tests of the referenced #1392 which is in master now. |
6 participants
See dlang/dmd#9275 (comment)