-
Notifications
You must be signed in to change notification settings - Fork 30
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
relax _include_this_registry
check
#426
base: master
Are you sure you want to change the base?
Conversation
I don't think we should apply this relaxed behavior by default. Perhaps we can provide a keyword argument, and when the user sets this keyword argument to a certain value, then we apply the relaxed behavior, but otherwise we apply the regular behavior. |
I have no idea what this code is good for. |
IIRC, we added |
Sorry, I still don't understand why there is an |
Sorry for the late review, but I agree with Gunnar that it looks like we create a fresh depot, then add the registries the user specifies, so it seems like we could just include them all and not have a check. What could the check filter out? |
Hmmmm..... it's been so long that I don't remember why the code is structured this way. Should we try changing the |
Sure |
Sometimes one might prefer to use a fork URL for various reasons(e.g., private network, slow connection to GitHub). The current
_include_this_registry
check fails silently by returning false. This causes some hard to figure error:This change introduces a
lowercase(basename(x))
alternative and uses the set intersection operation to simplify the logic.A debug log for easier review: