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

Return "no such user context" and "no such frame" instead "invalid argument" in "session.subscribe" and "session.unsubscribe" #865

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

lutien
Copy link
Contributor

@lutien lutien commented Jan 21, 2025

  • Return "no such user context" error instead "invalid argument" in "session.subscribe" command.
  • Return "no such frame" error instead "invalid argument" in "session.subscribe" and "session.unsubscribe" commands.

Preview | Diff

@lutien lutien marked this pull request as ready for review January 21, 2025 10:03
@lutien lutien requested review from jgraham and OrKoN January 21, 2025 10:03
index.bs Outdated
@@ -1143,7 +1143,7 @@ To <dfn>get related navigables</dfn> given an [=script/settings object=]

<div algorithm>

To <dfn>get navigables by ids</dfn> given a [=/list=] of context ids |navigable ids|:
To <dfn>get navigables by ids</dfn> given a [=/list=] of context ids |navigable ids| and |validate navigables|:
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work because if a function can return error it needs to return success in the case it doesn't fail.

If you made that change you'd also need to change every other caller to either pass in the correct value for the validate argument (or set a default) and to correctly unwrap the result (even if it's using |validate navigables| = false so it's defacto infallible).

All that said, I don't love the "here's an boolean parameter that totally changes the error handling of this algorithm" approach. I think I'd actually prefer to have two different functions, like get valid navigables by ids and get navigables by ids, with the former implementing the error handling on top of the latter in the same way we do it here (i.e. comparing set sizes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, alright. The only thing which I don't like is that then (at least for the implementation) we will have to reverse engineer which navigable ids belong to non-existent navigables to return a proper error message. So my suggestion would be to have two functions, as you said, but not use get navigables by ids in get valid navigables by ids and use get a navigable instead, which already does a validation. It's a bit more repetition, but I think a bit easier to implement. Let me know what you think. (The PR is updated to reflect the suggestion)

@lutien lutien force-pushed the validate-contexts branch from 389cd1c to af4218a Compare January 21, 2025 10:49
@lutien lutien merged commit da7ada3 into w3c:main Jan 21, 2025
5 checks passed
@lutien lutien deleted the validate-contexts branch January 21, 2025 13:26
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.

3 participants