Conversation
|
I don't get why it complains about logError in the travis build (a shell function I'm not even using) missing and why it even randomly broke there on these old compilers |
|
"logError" is called from "die" - my guess would be that the export for that is missing in "run-unittest.sh". But it's strange that this gets called at all, since the diff looks identical. |
s-ludwig
left a comment
s-ludwig
left a comment
There was a problem hiding this comment.
Apart from the stylistic nitpicks, the changes look good!
Have you tried running the added test case locally with an old compiler version? The diff output looks identical, and I thought that GitHub would mark any non-Linux line endings, so it's hard to tell from the distance what goes wrong there.
When the issues are resolved, this should also be rebased once before the merge.
source/dub/dependencyresolver.d
Outdated
| } | ||
| } | ||
|
|
||
| static struct ConfigEntry |
source/dub/dependencyresolver.d
Outdated
| } | ||
|
|
||
| CONFIG[] all_configs; | ||
| bool any_config; |
There was a problem hiding this comment.
Naming convention: allConfigs, anyConfig.
There was a problem hiding this comment.
I adapted to the original naming conventions of the file which used exactly such names
| Root package issue1037-better-dependency-messages reference gitcompatibledubpackage 1.0.1 cannot be satisfied. | ||
| Packages causing the conflict: | ||
| issue1037-better-dependency-messages depends on 1.0.1 | ||
| b depends on ~>1.0.2 |
There was a problem hiding this comment.
For better clarity of the "test" folder, this should be named "issue1037-expected-output".
There was a problem hiding this comment.
but all the other 'expected output files' are also named this way, it would be weird to only name this one like that
There was a problem hiding this comment.
Missed that. I'll change those too then.
|
Thanks for your pull request, @WebFreak001! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. |
|
ping on this? issue #1037 keeps popping up |
|
ping x10 |
|
@WebFreak001 I just msged you on slack |
|
@WebFreak001 i just rebased yours against master in #1377 |
|
got merged in #1377 |
5 participants
I grouped the arrays all_configs and any_config together into one struct for better maintainability and readability (and eventual segfaults where the index hasn't been checked)
Previously it only showed
Now it additionally shows