Conversation
|
How would the team feel about my deprecating |
|
The leading underscore indicates that symbol is private to the implementation. If that's the case, no objections here. |
|
Then again, if it's an implementation detail, then we don't have to deprecate it. We can just remove it. |
|
Ready for review. |
|
Spotted this in the type checking workflow. |
korydraughn
left a comment
There was a problem hiding this comment.
Looking good so far.
|
Sorry for the forced push... just put some ruff changes through and added a structure that maps all permission keys to their respective codes - including all synonyms. It will help someone that say wants to do this: which would sort the whole list of retrieved permissions and new applicable ACLOperations together, ordering by increasing numeric permission code. Yes, I know - very niche. But it affords the customer some pretty good flexibility. |
|
linters and tests are queued up. Code is ready for final review, I think. |
alanking
left a comment
There was a problem hiding this comment.
Looks like we're rounding the corner. Still see a couple unresolved comments
… casual corruption. RUF012 points out that instances of the class can casually modify an unmutable class variable, eg.: class A: value = [] def f(self,*y): self.value += [*y]
|
I think it's ready for final eyes.... Squashing. |
korydraughn
left a comment
There was a problem hiding this comment.
We're in the final stretch.
| from irods.path import iRODSPath | ||
|
|
||
|
|
||
| _ichmod_listed_permissions = ( |
There was a problem hiding this comment.
What's the reason behind including ichmod in the variable name?
There was a problem hiding this comment.
They were copy/paste from output of man ichmod. Definitely open to suggestions on that one.
| # Useful if we wish to force an explicitly specified local zone to an implicit zone spec in the copy, for equality testing: | ||
| if '' != implied_zone == other.user_zone: | ||
| other.user_zone = '' |
There was a problem hiding this comment.
Please explain L138.
I'm trying to guess what it does. Here's what I got (without looking up python docs) ... L138 can be rewritten as:
if '' != implied_zone and implied_zone == other.user_zone:
other.user_zone = ''Is my interpretation correct? What does it accomplish?
If implied_zone is tempZone and other.user_zone is tempZone, other.user_zone is cleared. implied_zone remains nonempty.
There was a problem hiding this comment.
That's the correct interpretation.
We're forcing other, ie the return value to have null length in the zone field if it matches a non empty "implied" value.
Any modification to the variable implied_zone would of course have no effect on any code external to the call frame. (That's just a python thing.)
|
|
||
|
|
||
| ( | ||
| _ichmod_synonym_mapping := { |
There was a problem hiding this comment.
Highlighting this for follow-up depending on the results of the other ichmod conversation.
| class _deprecated: | ||
| class _iRODSAccess_pre_4_3_0(_iRODSAccess_base): | ||
| codes = collections.OrderedDict( | ||
| (key.replace("_", " "), value) | ||
| for key, value in iRODSAccess.codes.items() | ||
| if key in ("own", "write", "modify_object", "read", "read_object", "null") | ||
| ) | ||
| strings = collections.OrderedDict((number, string) for string, number in codes.items()) | ||
| def __init__(self, *args, **kwargs): | ||
| warnings.warn( | ||
| "_iRODSAccess_pre_4_3_0 is deprecated and will be removed in a future version. Use iRODSAccess instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2 | ||
| ) | ||
| super().__init__(*args,**kwargs) | ||
|
|
||
| _deprecated_names = {'_iRODSAccess_pre_4_3_0':_deprecated._iRODSAccess_pre_4_3_0} | ||
|
|
||
| def __getattr__(name): | ||
| if name in _deprecated_names: | ||
| warnings.warn(f"{name} is deprecated", DeprecationWarning, stacklevel=2) | ||
| return _deprecated_names[name] | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
Does this do what's intended?
There was a problem hiding this comment.
I've tested it manually only. I wonder if a test is in order to make sure; wouldn't be that difficult.
| `<session>.permissions` was therefore removed in v2.0.0 | ||
| in favor of `<session>.acls`. | ||
|
|
||
| Atomic ACLs |
There was a problem hiding this comment.
Consider including "Permissions" in the title. That will make the section easier to find for some readers.
4c99900 to
968d9cd
Compare
No test yet, will undraft when I have one.
Exercised now with this script: