-
Notifications
You must be signed in to change notification settings - Fork 39
Update mdrepo.py #106
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
Update mdrepo.py #106
Conversation
This fix enables "bare" aliases to be succesfully retrievable via /alias requests. The main reason turns out to be an error in inlince doc in builtins.py.
|
Can you help me figure out why the test fails? I'm not ruling out the possibility that the test is broken or just failed randomly but I also wan't to make sure this fix doesn't break anything. |
|
I'm not a travis expert, but I can try. I just don't understand why a pull request with harmless comment does not trip on this as well? |
|
Thats actually not an error but stdout from a successful test. Look for "FAIL" |
|
The in test_alias_ndn PyFFDTest fails, as is to be expected. Back to the drawing board to see how we can discriminate between true and false positives for the strip("/"). Or, correct the inline documentation in builtins.py? |
Revert strip("/") fix.
Move strip("/") fix to store.py where a fallback check can be or'd with the original check. This makes distinction between /alias and alias impossible.
|
If you agree with the replacement of the strip("/") to store.py this PR looks fine now. |
|
Reviewing old PRs (sorry about the delay) - can you help me unconflict this PR and also perhaps provide a testcase that demonstrates the breakage? |
|
Changes Unknown when pulling 6e0d25d on mrvanes:patch-1 into ** on leifj:master**. |
|
It's clear from the docs now that the alias is actually a path and needs the leading / |
This fix enables "bare" aliases to be succesfully retrievable via /alias requests. The main reason turns out to be an error in inlince doc in builtins.py.