Skip to content
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

Fix room tags spec #1457

Merged
merged 4 commits into from
Jul 30, 2018
Merged

Fix room tags spec #1457

merged 4 commits into from
Jul 30, 2018

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Jul 27, 2018

The room tag spec started out as plain strings, then #1054 introduced namespaces but just added a paragraph without updating any of the existing copy to reflect namespaces, making the spec contradict itself.

  • The example given in the GET request didn't adhere to the namespaces. Also I found it a bit confusing as 'pinned' sounds like a specific supported feature.
  • Update copy to say they're namespaced
  • Remove references to deprecated API endpoints (and indeed non-deprecated ones as it shouldn't matter whether the client got the object via the HTTP C/S API or websockets or COAP or whatever).
  • Actually describe what the m.tag event looks like
  • Actually define what m.favourite and m.lowpriority mean...
  • Move namespace info up above the example to be with the rest of the definition.
  • Various grammar fixes

N.B. I'm trying to lead by example here by doing this a direct PR without a proposal doc, since I think this is all clarifying the existing intention of the spec rather than changing it.

@dbkr dbkr requested a review from a team July 27, 2018 11:25
any tags in this namespace they don't understand.
* The namespace ``u.*`` is reserved for user-defined tags. The portion of the string after the ``u.``
is defined to be the display name of this tag. No other semantics should be inferred from tags in
this namespace.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note change here defining the behaviour of stripping the u. to get the display name which I assume was what this intended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was indeed the original intent of #1054

Tags namespaces are defined in the following way, depending on how the client are expected to interpret them:

* The namespace ``m.*`` is reserved for tags defined in the Matrix specification. Clients must ignore
any tags in this namespace they don't understand.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change here: s.current/Matrix/ (the m. namespace is reserved for any version of the spec, not just this one)

@turt2live
Copy link
Member

This has been approved for a while - merging.

@turt2live turt2live merged commit f5af4d2 into master Jul 30, 2018
dbkr added a commit to matrix-org/matrix-react-sdk that referenced this pull request Aug 6, 2018
Fix Riot's behaviour with room tags after my cleanup in
matrix-org/matrix-spec-proposals#1457 . Although, reading
it again, it's not clear how you're supposed to tell the difference
between a reverse-dns tag name and a legacy freeform text tag
(contains a '.'?) - I've left it detecting these as freeform text
for now.
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this pull request Aug 22, 2023
This strives to fix all remaining cases where additional attributes
(most often 'description' but not only) are provided next to $ref
by wrapping $ref in allOf; and also drops allOf in a couple of places
where $ref is the only element under it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants