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

MSC2618: Helping others with mandatory implementation guides #2618

Open
wants to merge 2 commits into
base: old_master
Choose a base branch
from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jun 9, 2020

@turt2live turt2live changed the title MSC0000: Helping others with mandatory implementation guides MSC2618: Helping others with mandatory implementation guides Jun 9, 2020
@turt2live turt2live added kind:core MSC which is critical to the protocol's success proposal A matrix spec change proposal proposal-in-review process Related to the spec process itself (MSC process) labels Jun 9, 2020
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Great overall! I'm really looking forward to the clarity this may bring :)

Just a couple questions below.


MSCs which are self-documenting, such as the small/tiny maintenance ones, or which do not affect the
spec featureset, like this one, do not need to have implementation considerations documented. However,
it would be greatly appreciated if the redundancy was present for the sake of clarity.
Copy link
Member

Choose a reason for hiding this comment

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

By redundancy here, you mean that the MSC author should include some existing steps surrounding the feature they're modifying?

Copy link
Member Author

Choose a reason for hiding this comment

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

ish, yea. Basically if your MSC is "We should mandate that all text is purple" the implementation considerations should say "All text is now purple".

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see. This paragraph means redundancy between what you wrote and the implementation guide. I thought it was referring to redundancy with existing content in the implementation guide/other MSCs.

Anyone who plans on implementing an MSC should be prepared to document their issues for other implementations
to consider. This may add time to the development of a feature, however the greater ecosystem will benefit
more from the issues being recorded. Likewise, MSC authors should be made aware when their MSC is being
implemented and be prepared to document any common questions they receive as considerations.
Copy link
Member

Choose a reason for hiding this comment

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

Is this true even if the MSC has already been merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

merged or landed? If it's merged then we have a problem.

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not sure what the difference is between "merged" and "landed".

I mean the MSC itself has been merged into the proposals/ folder, a spec PR could still be absent.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is landed. Merged is a specific status for when a spec PR has been accepted and a spec release performed.

Copy link
Member

@anoadragon453 anoadragon453 Jun 11, 2020

Choose a reason for hiding this comment

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

Ahhh, ok, I see what you're saying now.

So... is this true even when an MSC has landed? :) I guessing this is true and someone's responsibility up until the MSC is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. It's no different than the requirement to keep the MSC updated with changes.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. I think this point and the other one could use some light clarification in the document? Otherwise the rest of it looks good to me.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Generally LGTM, except I think we should keep the spec as the cornerstone authority.


## Alternatives

We sit in sadness for lack of documentation, leading to frustrated developers and abandoned implementations.
Copy link
Member

Choose a reason for hiding this comment

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

On a more serious note, the alternative could be to continue filling the spec documents with implementation advices. The downside of that is described in the introduction, as this is business as usual to some extent.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. process Related to the spec process itself (MSC process) proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants