container clean ups#100
Merged
bloodearnest merged 5 commits intomainfrom Feb 22, 2024
Merged
Conversation
Previously, it was in PathItem, which was awkward, especially when making changes for file group tree view. This moves it to being an ephemeral attribute set on the container, and thus available to all PathIems. I added it as an attribute, so it can be set, and to do that I switched from a regular base class to `typing.Protocol`, which allows for specifying attributes. The original motiviation for using a base class (code reuse) had gone away previously
Previously, url paths mapped directly on to file paths, so using a full Path for both made a lot of sense. However, filegroups introduces a separation betweent the two. e.g. the absolute url path "/requests/view/REQUEST_ID/default/output/file" has a url path of `default/output/file` relative to the url root, but the file path of `output/file` relative to the directory root. So we use a renamed PurePosixPath as a convenient implementation of a UrlPath. This helps distinguish between the two types of paths via type annotations, but also prevents accidentally doing IO operations on the Pure path. Also added a convenient constant ROOT_PATH, which is the empty relative path.
Previously, this was done manually in the template. This was is self contained and better.
Previously, we had two separately named path for workspace and requests views. This gives them the same name, and provides a default value for path, as per recommend way in django docs. This allows us to consolidate our url handling to just one reverse call, that defaults to the root path if none is specified.
rebkwok
approved these changes
Feb 22, 2024
Contributor
rebkwok
left a comment
rebkwok
left a comment
There was a problem hiding this comment.
LGTM. Just some suggested changes to comments
Co-authored-by: Becky Smith <becky.smith@thedatalab.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A bevy of pre-factors and cleanups in prepartion for support filegroups in
navigation.