-
Notifications
You must be signed in to change notification settings - Fork 39
Don't store this field's default value in the store #109
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
Conversation
|
Thanks @TallTed, I've added Tim as a reviewer now. |
megoth
left a comment
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.
LGTM ^_^
|
Comments on #105 seem to make clear this is not "don't store a field's default value" but "don't store this field's default value" (or perhaps "don't store this app's user preference default value(s)"), which makes much more sense. |
|
@TallTed It's even "don't delete a field's default value from their Pod" - because that wouldn't work, because it's not in their Pod. The functionality you described would have to be implemented by the form that's using this field. (I only thought of that today; it was late for me yesterday. Sorry!) |
|
@TallTed The title is actually correct, I think: because the default value was stored in the rdflib Store (i.e. in-memory), it would then try to delete it from the Pod. So this PR stops it from adding it to the Store, which then results in it not trying to delete it from the Pod. (And the issue title identifies the bug that resulted from that behaviour.) Does that make sense? It's still nice if future readers can see this clarification in the PR comments :) |
|
I suggest changing the title because this is the first, and often only, thing people read when reviewing past issues/PRs. Reading over all the comments requires much more effort -- and is only likely to happen if the title suggests that it's about what they're researching. Title is now "Don't store a field's default value in the store" I suggested "don't store this field's default value in the store" You replied with "don't delete a field's default value from their Pod" which does seem most accurate thus far. Maybe "apps shouldn't try to save/delete a default field value to/from the user's pod/store" would be even better? |
|
@TallTed Oh, sorry - lost the context of your previous comment, which actually included a suggestion. Too many update notifications! I'll change it right away. |
Tim has said in a call that it's weird that this line is there in the first place and should be removed, so I guess this doesn't need another review from him.
Fixes #105.