-
-
Notifications
You must be signed in to change notification settings - Fork 331
Create read only copy if needed when opening a store path #3156
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
base: main
Are you sure you want to change the base?
Conversation
case _: | ||
raise ValueError(f"Invalid mode: {mode}") |
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.
I moved the mode checking up to the start of the function.
Co-authored-by: Davis Bennett <[email protected]>
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.
Looks good to me. Requesting changes because:
- This should get a release note
- My old release note from when I added the error should be deleted
@@ -39,6 +40,7 @@ | |||
JSON = str | int | float | Mapping[str, "JSON"] | Sequence["JSON"] | None | |||
MemoryOrder = Literal["C", "F"] | |||
AccessModeLiteral = Literal["r", "r+", "a", "w", "w-"] | |||
ANY_ACCESS_MODE: Final = "r", "r+", "a", "w", "w-" |
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.
ANY_ACCESS_MODE: Final = "r", "r+", "a", "w", "w-" | |
ANY_ACCESS_MODE: Final = get_args(AccessModeLiteral) |
It might be possible to de-duplicate here (untested) using typing.get_args
?
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.
at least on my machine, the type inference machinery thinks get_args(...)
returns tuple[Any,...]
. So this won't work.
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.
Does it matter what the type is of ANY_ACCESS_MODE
though, since it's only used for runtime checking? Not a big deal given these two lines of code are next to each other though, so should be obvious if they end up out of sync.
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.
it does matter, because when you annotate a tuple of strings as Final
, type checkers will know that it's Literal[*stuff_in_the_tuple]
, which means you can use that variable for "compile-time" and runtime type checks.
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.
It worked on my machine and I was curious what the CI would say so I pushed 7ad760f
(#3156) (would be easy to revert 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.
CI will pass because the return type of get_args()
is tuple[Any,...]
, and so any type is assignable to its contents.
lit_typ = Literal["a", "b"]
lit_val: Final = ("a", "b")
x: lit_typ = lit_val[0]
# the type of x is inferred to be Literal["a"]
y = get_args(lit_typ)[0]
# the type of y is inferred to be Any
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.
another argument against get_args
here:
y: lit_typ = get_args(lit_val)[15]
# mypy is OK with this
z: lit_typ = lit_val[15]
# mypy sees the indexing error
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.
thanks for the explanation, I reverted the change in a1949f1
(#3156).
It seems like our options are redundancy or using get_args
inside StorePath.open()
. @d-v-b previously suggested that the redundancy option offers minor performance benefits.
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.
yeah it's a handful strings, so writing it down twice in essentially the exact same location is not expensive. If we are really worried that the type and the final variable might diverge, we can test for that (using get_args).
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.
yeah it's a handful strings, so writing it down twice in essentially the exact same location is not expensive. If we are really worried that the type and the final variable might diverge, we can test for that (using get_args).
this seems like a reasonable compromise. I added a test in 202d606
(#3156).
Co-authored-by: David Stansby <[email protected]>
Co-authored-by: David Stansby <[email protected]>
Thanks, done in |
This PR makes a read-only copy of a store when using
StorePath.open(mode="r")
.See discussion in #3068. This augments #3068 (it still errors when attempting to open a writeable store path using a read-only store). This is an alternative to #3155.
Closes #3147
TODO:
docs/user-guide/*.rst
changes/