Proxito: adapt unresolver to make it usable for proxito #10037
Conversation
stsewd
force-pushed
the
refactor-unresolver
branch
from
215718e to
4f10484
Compare
stsewd
force-pushed
the
refactor-unresolver
branch
2 times, most recently
from
fd683dd to
9186199
Compare
stsewd
force-pushed
the
refactor-unresolver
branch
from
9186199 to
c505544
Compare
|
This is ready for review. |
| # We don't call unparse() on the path, | ||
| # since it could be parsed as a full URL if it starts with a protocol. | ||
| parsed_url = ParseResult( | ||
| scheme="", netloc="", path=path, params="", query="", fragment="" | ||
| ) |
There was a problem hiding this comment.
Shouldn't we validate that path comes in the exact shape we require here?
There was a problem hiding this comment.
Not sure if I understand what you mean with shape, anything can be a path, this is the path as we receive it from proxito.
There was a problem hiding this comment.
We are expecting a string with a particular structure. For example, it can't be https://humitos.dev/page/, it has to be just /page/. I'm saying that we should validate the path to be what we expect to be instead of a random string.
There was a problem hiding this comment.
what we expect is a random string
There was a problem hiding this comment.
In the code comment you said that it should not start with a protocol, tho. I'm confused what are the correct values for path then. Can you expand on this?
There was a problem hiding this comment.
The comment says we can't use unparse() because the path could start with a protocol, or anything really that can confuse unparse() so we manually build the ParseResult class.
There was a problem hiding this comment.
Yeah, I got that. I'm not saying we have to use unparse(). I'm saying that we can validate the string to be exactly in the shape we need to be and raise an exception otherwise.
| :returns: None or a tuple with the current project, version and file. | ||
| A tuple with only the project means we weren't able to find a version, | ||
| but the translation was correct. | ||
| :returns: A tuple with the current project, version and file. |
There was a problem hiding this comment.
Shouldn't this be an object? Maybe an Unresolved<Something> object to keep consistency with the other objects.
There was a problem hiding this comment.
This is for internal usage only, they are always present.
There was a problem hiding this comment.
That's fine. Even if it's internal. Using well-known objects makes the code easier to read and follow.
There was a problem hiding this comment.
This 3 tuple elements is well-defined, I'd avoid creating data classes for every return type, if we have more than 3 things to return, then that would make sense for me.
Instead of returning a partial response, I'm just raising an exception with the information from the context where it happened, this will be used in proxito to decide what to do (redirect, raise 404, etc)