-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Allow registering absolute URLs for autorefs #8
Conversation
For now this is not used for anything, but the refactor is good for whatever plan we decide to go through regarding inventories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment.
if fallback: | ||
new_identifier = fallback(identifier) | ||
if new_identifier: | ||
return self.get_item_url(new_identifier, from_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we risk infinite recursion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry that there's no explanation for the "cleverness".
The self-call is not exactly the same, fallback
is dropped. So, no, there will be only 2 attempts.
I'll add an explicit fallback=None
for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad for not reading this more thoroughly, I would have seen it 🙂
Thanks for the explanation. The fallback=None
is not necessary, but you can add it if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you did. Thanks again ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh let me revert that, there's some failure, and it's a concern for backwards compatibility.
I'll also check in detail why the test failed, I would not have expected that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is resolved btw. There wasn't really a problem, just an implicit merge conflict
For now this is not used for anything, but the refactor is good for whatever plan we decide to go through regarding inventories.