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

Inline RTCSessionDescriptionInit description #33963

Merged
merged 10 commits into from
Sep 23, 2024

Conversation

bc-lee
Copy link
Contributor

@bc-lee bc-lee commented Jun 6, 2024

Description

Add missing explanation of RTCSessionDescriptionInit and RTCLocalSessionDescriptionInit dictionaries. Also correct several links referencing those interfaces.

Motivation

Additional details

Related issues and pull requests

Fixes #33962

…lSessionDescriptionInit

Add missing documentation for RTCSessionDescriptionInit,
RTCLocalSessionDescriptionInit interfaces. Also correct several links
referencing those interfaces. Also add a clear explanation of the
relationship between RTCSessionDescription, RTCSessionDescriptionInit,
and RTCLocalSessionDescriptionInit interfaces.

Fixes mdn#33962
@bc-lee bc-lee requested a review from a team as a code owner June 6, 2024 12:53
@bc-lee bc-lee requested review from wbamberg and removed request for a team June 6, 2024 12:53
@github-actions github-actions bot added Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Jun 6, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@skyclouds2001 skyclouds2001 left a comment

Choose a reason for hiding this comment

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

I think the two dictionaries are better not to placed in separate pages, as they both only contains two properties and both only used in one method

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Hi! Unfortunately we don't document dictionaries on MDN, unless they are reused by a lot of methods (and even in such case we would usually just copy the property list around). Could you inline the description in the places where they are used instead?

Combine RTCSessionDescriptionInit and RTCLocalSessionDescriptionInit
descriptions within the RTCSessionDescription page.
Additionally, update any related links.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@bc-lee bc-lee requested a review from Josh-Cena June 7, 2024 19:27
Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

We never mention the dictionary's name at all, because it's not a code entity. It should look like this:

  - : An object with the following properties:
    - `property1`
      - : What it does

And then for the errors, you would just say "the sessionDescription parameter is invalid".

@bc-lee
Copy link
Contributor Author

bc-lee commented Jun 8, 2024

Do you mean I should update the PR to remove any references to the directory name RTCSessionDescriptionInit? It existed before I submitted my PR.

@Josh-Cena
Copy link
Member

Yes, they should be removed.

@bc-lee
Copy link
Contributor Author

bc-lee commented Jun 10, 2024

I'll keep my PR open for now, but I'm not sure if I can make any more changes, especially considering that the distinction of dictionary names is required since they are part of the WebRTC API. I believe it is important to retain the dictionary name in the documentation. If someone wants to take over the PR, I'm happy to close it.

@Josh-Cena
Copy link
Member

Please see past changes where dictionaries were removed, for example #30080, #30079

@hamishwillee
Copy link
Collaborator

hamishwillee commented Jun 10, 2024

@bc-lee FYI @Josh-Cena is correct - the fact that the spec mentions these dictionaries is irrelevant. Specs are written for browser developers, while MDN interprets the specification for webapp developers.

In most cases we've found that having separate dictionaries is unhelpful for web app developers - they have to jump around to find relevant information, and often the compatibility data for the dictionary depends on where it is used. Because of that, the best way to properly track compatibility is to expand the dictionary.

We're not dogmatic about it. For some APIs it does actually make sense to define the objects passed to APIs as dictionaries. Generally this is when you have a general API that can take numerous possible objects depending on the context in which it is used.
This is not the case for WebRTC.

I haven't looked at this closely, but based on the comments we won't be merging this as is. I'm sure it will be useful though if modified to match MDN conventions.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Jun 11, 2024
@bc-lee
Copy link
Contributor Author

bc-lee commented Jun 13, 2024

Two days ago, a pull request relevant to this one (#34051) was merged. I'd like to hear from @Josh-Cena and @hamishwillee on whether you believe the merged PR achieves the same level of consistency as requested in this pull request.

@Josh-Cena
Copy link
Member

Josh-Cena commented Jun 13, 2024

@bc-lee I'm not sure what you want to express? That PR corrects incorrect information to correct information. While it doesn't remove documentation for dictionaries, that's fine because it's (a) making a correctness improvement, which is more important than editorial conventions (b) not adding more documentation pages for dictionaries.

Of course it would be better if that PR simply removed the mention to RTCSessionDescriptionInit altogether and just said "the return value is a promise resolved to an object with the following properties", but that's not necessary to qualify as a correctness PR. This PR on the other hand tries to introduce more documentation about a dictionary.

@Josh-Cena
Copy link
Member

@bc-lee, do you have time to come back to this?

@bc-lee
Copy link
Contributor Author

bc-lee commented Aug 12, 2024

Yes, but not today or tomorrow (atm I'm busy with other things). I'll come back to this in a few days.

#33955 (comment)

@Josh-Cena Josh-Cena added the awaiting response Awaiting for author to address review/feedback label Aug 27, 2024
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Sep 23, 2024
…x.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Josh-Cena Josh-Cena changed the title fix: Add missing documentation for RTCSessionDescriptionInit, RTCLocalSessionDescriptionInit Inline RTCSessionDescriptionInit description Sep 23, 2024
Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

I've simply inlined the descriptions in each page and reordered some sections. I did not do other major changes. Thanks for the PR!

@Josh-Cena Josh-Cena merged commit b913cec into mdn:main Sep 23, 2024
8 checks passed
@bc-lee bc-lee deleted the feature/rtcsessiondescriptioninit branch September 23, 2024 05:30
fiji-flo pushed a commit that referenced this pull request Oct 2, 2024
* fix: Add missing documentation for RTCSessionDescriptionInit, RTCLocalSessionDescriptionInit

Add missing documentation for RTCSessionDescriptionInit,
RTCLocalSessionDescriptionInit interfaces. Also correct several links
referencing those interfaces. Also add a clear explanation of the
relationship between RTCSessionDescription, RTCSessionDescriptionInit,
and RTCLocalSessionDescriptionInit interfaces.

Fixes #33962

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Apply Code Review

Combine RTCSessionDescriptionInit and RTCLocalSessionDescriptionInit
descriptions within the RTCSessionDescription page.
Additionally, update any related links.

* Update files/en-us/web/api/rtcsessiondescription/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update files/en-us/web/api/rtcsessiondescription/rtcsessiondescription/index.md

* Update

* Update files/en-us/web/api/rtcpeerconnection/setlocaldescription/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Fix

* Fix others

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joshua Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Awaiting for author to address review/feedback Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken link to RTCSessionDescriptionInit, RTCLocalSessionDescriptionInit
4 participants