-
Notifications
You must be signed in to change notification settings - Fork 81
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
delTufoProp implementation and events/splices #358
Conversation
…ng ro props during add
def _actNodePropDel(self, mesg): | ||
form = mesg[1].get('form') | ||
valu = mesg[1].get('valu') | ||
prop = mesg[1].get('prop') |
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.
If we can set multiple props at once, should we be able to delete multiple props at once?
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.
My comment above does not need to be addressed as part of this PR.
See: #359
if pdef[1].get('ro'): | ||
raise CantDelProp(name=prop, mesg='property is read only') | ||
|
||
# if the prop has a default value, it may not be deleted |
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.
If I form a node and set this prop to a non-default value at formation time, then call delTufoProp on it, should Synapse really say that I can't delete it? Would it be weird to set it back to its defval?
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.
10 seconds later... I think that letting the user delete the prop would lead to unintended problems later on, where other users would expect a value or defval. Also, I think that del resetting it to default probably doesn't make sense either, as the user should explicitly reset it to default.
@@ -77,6 +77,9 @@ class BadCoreStore(SynErr): | |||
'''The storage layer has encountered an error''' | |||
pass | |||
|
|||
class CantDelProp(SynErr): pass | |||
class CantSetProp(SynErr): pass |
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.
Unused
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.
Probably want to make setTufoProps raise it if prop is ro
/etc.
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 think this will be difficult to do until #359 is complete
causes a slight splice change. splice action node:set has been updated to node:prop:set to make a namespace which can also include node:prop:del ( since node:del is already something else )