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

Resolve sort_by specification issue #488

Merged
merged 4 commits into from
May 22, 2023
Merged

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented May 19, 2023

Resolves #480

  • Proposes change that allows field to sort_by to be specified by either a top-level field name or a JSON pointer (with RFC citation)
  • an implementation could support both, since JSON pointers will always have a leading slash in this context, so if there is no slash, it can be interpreted as a field name... unless a field name starts with a slash. Don't do that.
  • Should cover all existing implementations so testing data should still be valid
  • We should clean this up in the next version of the Discovery spec. In particular we should specify that both should be supported by all implementations to avoid interoperability issues, or just allow one (like JSON Pointers), but that would go too far at this point (non-editorial, impacting implementations, etc).

Preview | Diff

Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

This is a big feature drop hidden inside an assertion. It doesn't put the assertion at risk because other implementations implement the other methods prescribed in the same assertion.

IMO, this should be a separate assertion and have two separate backing implementations.

took out the entire custom sorting section
remove sort_by and sort_order options
can't nest comment
@mmccool
Copy link
Contributor Author

mmccool commented May 22, 2023

Discussed in Discovery call May 22, felt that testing was not consistent and it was not reasonable to conclude that this was "tested", not to add additional specification e.g. JSON pointer. Several related at-risk assertions also had to be removed if this one is - in addition to one that was NOT at risk going into CR, but that was about returning an error if the feature was not supported (which is non-sensical to include if there is no feature). This PR now comments out the entire section and makes related repairs to the TM and other mentions of custom sorting. Also, sorting by ID was specified as being in ascending order, which was implied by the default value of the now-removed sort_order parameter.

@mmccool mmccool merged commit 399e337 into w3c:main May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sort_by should be a JSON pointer
2 participants