-
-
Notifications
You must be signed in to change notification settings - Fork 433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: combine aliases on windows base dirs (ie: X:\
) (fixes: #577)
#1080
Conversation
@vaab Thanks for sticking with this! I took a quick look at the change, and it seems straightforward enough. The tests are passing here on GitHub, so it seems good. If you could add a test of the specific problem, that would be great! |
Codecov Report
@@ Coverage Diff @@
## master #1080 +/- ##
==========================================
- Coverage 91.30% 91.24% -0.06%
==========================================
Files 90 90
Lines 13358 13358
Branches 1493 1493
==========================================
- Hits 12196 12188 -8
- Misses 951 959 +8
Partials 211 211
|
Added the test as required, kept it quite simple. Thanks for your feedback. Should I worry about the coverage drop ? It seems to happen in a test file, which was seemingly untouched by my change. |
@vaab no worries about the codecov drop. Ironically, I can't make heads or tails of those reports, and have turned them off. Thanks! |
This is now released as part of coverage 5.4. |
Illustrating a valid fix for my issue, that is still happening ;-)
I had a (very quick) look at running the tests, but unfortunately I had a lot of tests failing on the pristine source prior to any change. So I figured I missed a lot of dependencies or information to get this running properly. I'm afraid that I couldn't check that I didn't break anything properly before submitting that code change.
If you think this code change is definitively worth it, I'll take the time to add tests to your convenience if required.