-
Notifications
You must be signed in to change notification settings - Fork 123
XEP-0492: Rework whole spec and fix schema #1478
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
2162d7c to
4c1fdfa
Compare
xep-0492.xml
Outdated
| <p>Finally, clients can use this specification to synchronise finer-grained notification settings using custom namespaces.</p> | ||
| <example caption='An example of notification settings by client type'><![CDATA[ | ||
| <p>Finally, clients can use this specification to synchronise finer-grained notification settings using custom namespaces in the optional <advanced/> child of the notification elements.</p> | ||
| <p>The <advanced/> element MUST NOT be empty</p> |
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.
Isn't MUST NOT a bit too much for this? What is the problem with an empty <advanced> element besides uselessness?
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.
Well, one thing I don't like is that our specs often are too lax and in hindsight all sorts of incompatibilities arise. So I for myself adopted a rule of being as strict and clear as possible.
But of course you are right, an empty <advanced/> should not be harmful here. Do you want me to change that MUST to a SHOULD or remove it entirely?
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.
Let's make it SHOULD NOT
xep-0492.xml
Outdated
| <li>Matching '<tt>identity-category</tt>' attribute</li> | ||
| <li>Fallback element without '<tt>identity-category</tt>' and '<tt>identity-type</tt>' attributes</li> | ||
| </ol> | ||
| <p>When writing the notifiction settings, applications MAY use the '<tt>identity-category</tt>' and '<tt>identity-type</tt>' attributes as described in the <link url="#identities">Identities</link> section. If a fallback element without any attributes isn't present, the application SHOULD add it with the same settings. As secondary fallback, it MAY also add a notification setting having only the '<tt>identity-category</tt>' attribute, if not already present.</p> |
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.
typo: notifiction →notification
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.
thx :)
|
With my editor hat on: Are you fine with those changes @truenicoco? Should I go ahead and merge this? |
After the first comment (about the empty |
4c1fdfa to
75ec3cf
Compare
|
I've force pushed the SHOULD NOT and typo correction. |
All good to me now. |
As discussed in chat, rendered html: xep-0492.html