search

opensafely-core / airlock

Other
1 stars 0 forks source link

Refactor UrlPath #622

Open madwort opened 4 weeks ago

madwort commented 4 weeks ago

There are times when a UrlPath means "path to file in a workspace" and times when it means "actual url" and times where it means "request url that will include group". cf bll.get_request_file_from_output_path and bll.get_request_file_from_urlpath.

Previously, I tried to split those into separate types: OutputPath, UrlPath, RequestUrlPath (which had a .group as you suggest and a .output_path w/o the group), and use the right types to help us catch these distintions.

But at the time, I couldn't make it work cleanly. It's probably worth trying again, and definitely would be a good refactor.

https://github.com/opensafely-core/airlock/pull/621#discussion_r1718240397

madwort commented 4 weeks ago

I think I've fought this a bit in the past (having to keep in mind whether a path contains a group name or not) - would be great to have it really obvious (& enforced by the type system).

bloodearnest commented 3 weeks ago

One idea I wanted to try here was have the types enforced/converted in our django urls.py patterns, so that the views got typed instances of UrlPath/GroupPath, rather than strings.

You can do this with custom url type converters easily enough: https://docs.djangoproject.com/en/5.0/topics/http/urls/#registering-custom-path-converters