Get rid of Url.package_with_version #952
Conversation
src/ocamlorg_frontend/url.ml
Outdated
| let with_version = Option.fold ~none:"/latest" ~some:(fun v -> if v = "" then v else "/" ^ v) | ||
|
|
||
| let package_with_version ?version ?hash v = | ||
| with_hash hash ^ "/" ^ v ^ "/" ^ with_version version | ||
| let package ?version ?hash v = | ||
| with_hash hash ^ "/" ^ v ^ with_version version |
There was a problem hiding this comment.
IMO all the URLs that only redirect somewhere else should be separate from those intended for use in templates.
So, for URLs used in the templates and as redirection target, we should always require either a version or "latest".
There was a problem hiding this comment.
Yes, I had something along those lines in mind as a second stage/commit. It makes sense to use an optional argument
- Avoids writing Some
- Avoids passing None
Since there are three cases: specific, latest and not specified; there are two possible implementations:
- None for latest and Some "" for not specified
- None for not specified and Some "latest" for latest
I picked the first because it was the implicit one earlier. Do you thing we should go for the second or do you have another idea?
There was a problem hiding this comment.
No, I think we should make sure that we never use /p/PACKAGE-URLs in the templates and that this also cannot happen by accident.
So, there should be:
- one function that renders a package URL (which always is either a versioned URL
/p/package/VERSIONor latest-URL/p/PACKAGE/latest) - one redirect /p/PACKAGE -> /p/PACKAGE/latest
There was a problem hiding this comment.
No, I think we should make sure that we never use
/p/PACKAGE-URLs in the templates and that this also cannot happen by accident.
Then we need to update dependencies and conflicts in package overview, it's happening there :-)
- one function that renders a package URL (which always is either a versioned URL
/p/package/VERSIONor latest-URL/p/PACKAGE/latest)
Let's see what I can do.
* Remove old Url.package * Rename old Url.package_with_version into Url.package * Update Url.with_version - None -> "/latest" (as in old Url.package_with_version) - Some "" -> "" (as in old Url.package) - Some x -> "/x" (as in old Url.package_with_version) * Use new Url.with_version in new Url.package * At call sites of old Url.package, add ~version:"" to get the same behaviour * At call sites of old Url.package_with_version, replace by Url.package A bit convoluted, but did not managed to observe a change of behaviour.
cuihtlauac
force-pushed
the
url-tweaks
branch
from
ed91474 to
379c200
Compare
|
Subsumed by #999 |
2 participants
same behaviour
A bit convoluted, but did not managed to observe a change of behaviour.