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

Add PR template #100

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Add PR template #100

merged 8 commits into from
Aug 8, 2024

Conversation

sanketj
Copy link
Member

@sanketj sanketj commented Jun 20, 2024

This PR introduces a PR template that all subsequent changes into this repo should use, as per resolution to w3c/editing#463. Once finalized, this template will be added to other Editing WG repos as well.

This PR introduces a PR template that all subsequent changes into this repo should use, as per resolution to w3c/editing#463. Once finalized, this template will be added to other Editing WG repos as well.
@sanketj
Copy link
Member Author

sanketj commented Jun 20, 2024

@whsieh I wasn't able to add you as a reviewer for some reason, but please review this PR if you have any thoughts.

@sanketj sanketj requested a review from saschanaz June 20, 2024 22:14
Copy link
Member

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

This matches w3c/clipboard-apis#215, 👍 based on that.

The initial Closes #???? part is missing here btw, is it intentional? 👀

@johanneswilm
Copy link

Looks good. But I wonder if it creates too much overhead if you need to specify which those two browser engines are. It means you may have to email people working gon other teams to ask if you can add them, etc. Maybe we can combine the two checkpoints to:

 * [ ] Editing WG resolution on the proposed changes with at least two implementers participating and agreeing.

I don't think we have ever had a meeting without at least one implementer present. And those meetings with just one implementer present were at the very beginning when things were relatively chaotic. For the last many years we have had two if not three implementers present. Specifying the implementers additionally could nevertheless be seen as a security measure that no meetings are being held without implementer participation and then used to drive through big changes.


Can we also have a checkbox "Changes are not normative" or some such thing so that we can make spell fixes and similar without needing to go through this?

@sanketj
Copy link
Member Author

sanketj commented Jun 20, 2024

This matches w3c/clipboard-apis#215, 👍 based on that.

The initial Closes #???? part is missing here btw, is it intentional? 👀

Thanks for reviewing. I based this off of https://github.com/whatwg/html/blob/main/PULL_REQUEST_TEMPLATE.md?plain=1, which doesn't include Closes #????, and including that also wasn't explicitly part of the resolution here: w3c/editing#463 (comment). I'm personally okay with that part not being in the template, but happy to include it if folks feel strongly about it.

@saschanaz
Copy link
Member

saschanaz commented Jun 20, 2024

Looks good. But I wonder if it creates too much overhead if you need to specify which those two browser engines are. It means you may have to email people working gon other teams to ask if you can add them, etc. Maybe we can combine the two checkpoints to:

If one knows a PR has support from two implementors, one should also know which implementors do. If one can't name them then I would say one doesn't really know whether there's enough agreement.

Recording implementor agreement would also help telling which implementor did not agree.

Can we also have a checkbox "Changes are not normative" or some such thing so that we can make spell fixes and similar without needing to go through this?

Maybe a comment like <!-- Please fill the below when making normative changes. If it's not normative, feel free to remove them --> might work.

@johanneswilm
Copy link

The resolutions of our meetings are usually shown in our Github issues. If the chatbot isn't used for a particular meeting, the meeting notes can be found in the publically available public-editing-tf email list archive. I think it would make sense to link to the resolution to merge this in some place. So either an issue number or just a regular link. That will also make it easier for others to go back to the discussion later and figure out why we decided to go for a particular proposal.

@saschanaz
Copy link
Member

Having a corresponding link for meeting note is a good idea, but if it's clear from the meeting note, then one would be able to easily name them in the PR?

@johanneswilm
Copy link

johanneswilm commented Jun 20, 2024

If one knows a PR has support from two implementors, one should also know which implementors do. If one can't name them then I would say one doesn't really know whether there's enough agreement.

All the decision go through the monthly calls in which all the implementers are present. But oftentimes/mostly we discuss until I or someone else asks "Sounds like we found an agreement. Is anyone against resolving on X?" and if no one speaks, then X is the resolution. So its basically always all three engines. It's a bit of overhead to then additionally have to go back to implementers after the meeting and ask "Can I put your name on the PR?". But if we can say "you have to speak up during the meeting if you don't want to have your browser engine mentioned as in agreement" then maybe we can skip that part.

However, if we do that, it's also kind of meaningless to mention specific implementers at all.

If there are three implementers of a specific specification, we don't usually go ahead making a change against one one of the implementers. Worst case, if one engine is not willing to implement something specific and the other two do, is that we split a specification into several levels and then get consensus from everyone on that split as being the best possible outcome.

I'm not really sure it's a good idea to start with two vs one decisions now.

Maybe a comment like <!-- Please fill the below when making normative changes. If it's not normative, feel free to remove them --> might work.

That sounds good.

@saschanaz
Copy link
Member

It's a bit of overhead to then additionally have to go back to implementers after the meeting and ask "Can I put your name on the PR?".

I think what I don't underestand is particularly this part. If you already had a meeting with them I don't think that step is needed. You'll have some supporters before that "does anyone disagree?" step, then you can just name them (or better, @ them so that they can know), given it's all documented in the notes?

@sanketj
Copy link
Member Author

sanketj commented Jun 20, 2024

However, if we do that, it's also kind of meaningless to mention specific implementers at all.

Right. If we are going that route, then the "At least two implementers are interested" checkbox isn't meaningful.

I see these checkboxes as sanity checks to prevent (1) two or more implementers agreeing but not getting formal resolution as part of the WG process and (2) WG taking a resolution without two or more implementers agreeing. I think neither is likely to happen, but having separate checkboxes helps enforce that we have truly achieved both.

Ideally, we achieve both during the same discussion at our Editing WG call. If so, the "implementor interest" sections can just link to meeting minutes.

@johanneswilm
Copy link

It's a bit of overhead to then additionally have to go back to implementers after the meeting and ask "Can I put your name on the PR?".

I think what I don't underestand is particularly this part. If you already had a meeting with them I don't think that step is needed. You'll have some supporters before that "does anyone disagree?" step, then you can just name them (or better, @ them so that they can know), given it's all documented in the notes?

Sure, that's what I meant was one option. That we don't ask for extra permission to put engines on as "sponsors" of a PR and we can just do it if they didn't protest when asked whether they agree with the resolution in the meeting. However, the result is then that all three engines will be mentioned in every PR.

If we really want to mention the engines in the PR, maybe we can just put the name all three engines in the template and if there is ever a case where one of the engines decided to go against a decision or decided not to show up to the meeting, then one can remove the name in that particular PR.

@sanketj
Copy link
Member Author

sanketj commented Jun 20, 2024

It's a bit of overhead to then additionally have to go back to implementers after the meeting and ask "Can I put your name on the PR?".

I don't think it is necessary to require PR review from the individual that expressed interested. (Not a bad thing to do, but doesn't need to be required.) The way I see that section being filled out is, as an example:

 * [x] At least two implementers are interested:
   * WebKit: [link to comment/minutes that express interest]
   * Chromium: [link to comment/minutes that express interest]
   * Gecko: [link to comment/minutes that express interest]

If it is helpful, I can update the template to look more like that and I can add checkboxes for each of the engines. What do you think?

@saschanaz
Copy link
Member

In general I don't think listing each support is meaningless, even if all PRs end up getting all support. (That's a great thing!)

Mentioning all three from the template and linking for each sounds good to me.

@johanneswilm
Copy link

johanneswilm commented Jun 20, 2024

I see these checkboxes as sanity checks to prevent (1) two or more implementers agreeing but not getting format resolution as part of the WG process and (2) WG taking a resolution without two or more implementers agreeing. I think neither is likely to happen, but having separate checkboxes helps enforce that we have truly achieved both.

Ok, we want the same thing here. I can live with two checkboxes if that makes it clearer to others.

Let's make it clear that it is enough to have one WG resolution of a meeting where two implementers were present and didn't protest against the resolution to merge this and that there is no need to go back to get more than that. For example have two checkboxes like this:

[ ] An Editing WG resolution,
[ ] taken by a WG meeting with the representatives of at least two browser engines present and agreeing

Github issue # or link to meeting minutes: [[LINK]]

And do we really just want to have two implementers agreeing in case of specs implemented in all three engines? That's not how we have been operating so far.

I don't think we need individual links to decisions by each individual browser engines as that would crate more overhead. Do we even have such links for each change?

Add checkboxes of each implementor under the "At least two implementers are interested" section.
@johanneswilm
Copy link

At least two implementers are interested

I don't think we need each browser engine to make a formal decision that are interested or in favor of every change every time. From my experience, getting official approval from the higher ups at the browser engines is not always needed and can make everything really slow. The important part is not that everyone said "yes" and that that is in the minutes, but rather that no-one said "no". You will know yourself best at what stage you will need to have formal approval for a specific change and when it is not required.

In those cases where you need formal approval and you are not sure whether you will get it, we normally postpone a decision to a later meeting.

@saschanaz
Copy link
Member

And do we really just want to have two implementers agreeing in case of specs implemented in all three engines? That's not how we have been operating so far.

I think in that case it's safe to assume support.

@saschanaz
Copy link
Member

The important part is not that everyone said "yes" and that that is in the minutes, but rather that no-one said "no".

Even if it ends up implemented by no one? A spec should assume some interest, no?

@sanketj
Copy link
Member Author

sanketj commented Jun 20, 2024

I see these checkboxes as sanity checks to prevent (1) two or more implementers agreeing but not getting format resolution as part of the WG process and (2) WG taking a resolution without two or more implementers agreeing. I think neither is likely to happen, but having separate checkboxes helps enforce that we have truly achieved both.

Ok, we want the same thing here. I can live with two checkboxes if that makes it clearer to others.

Let's make it clear that it is enough to have one WG resolution of a meeting where two implementers were present and didn't protest against the resolution to merge this and that there is no need to go back to get more than that. For example have two checkboxes like this:

[ ] An Editing WG resolution,
[ ] taken by a WG meeting with the representatives of at least two browser engines present and agreeing

Github issue # or link to meeting minutes: [[LINK]]

As phrased, that doesn't really leave any room for async resolutions. I know we don't do too many of those, but given that we only meet once a month, I think we should at least allow those to happen (as they did before we starting meeting at all).

@johanneswilm
Copy link

johanneswilm commented Jun 20, 2024

And do we really just want to have two implementers agreeing in case of specs implemented in all three engines? That's not how we have been operating so far.

I think in that case it's safe to assume support.

Right, I mean we could in theory go on with just two implementation and break the web for the last implementation. But over the past decade, that's not what we have done. Last time we had such a situation was around 2017 or so when suddenly Chromium could not implement the full Input event spec. And even then did we not just split and instead we came to agreement on two levels of the Input Events spec and had consensus on that.

The point of having these templates should not be that we start working fundamentally differently or go away from building consensus on changes in the editing space. It's supposed to be a help to make sure that bugs are filed with all three implementers. The rest of it seems less helpful, especially if we now suddenly start adding things to spec against the wishes of one of the implementers.

@johanneswilm
Copy link

As phrased, that doesn't really leave any room for async resolutions. I know we don't do too many of those, but given that we only meet once a month, I think we should at least allow those to happen (as they did before we starting meeting at all).

For first few years we had a lot of meetings over email lists. but it's harder to get consensus that way. So yes, we have switched to go through these monthly calls (a request by Microsoft back in the day). As it is now, I don't really see how you would make such asynchronous decisions - How would that work? First a monthly call in which implementers do not participate and later another meeting in which only implementers participate?

Add header comment about non-normative changes.
@sanketj
Copy link
Member Author

sanketj commented Jun 20, 2024

As phrased, that doesn't really leave any room for async resolutions. I know we don't do too many of those, but given that we only meet once a month, I think we should at least allow those to happen (as they did before we starting meeting at all).

For first few years we had a lot of meetings over email lists. but it's harder to get consensus that way. So yes, we have switched to go through these monthly calls (a request by Microsoft back in the day). As it is now, I don't really see how you would make such asynchronous decisions - How would that work? First a monthly call in which implementers do not participate and later another meeting in which only implementers participate?

The CSS WG, for example, have a system for async resolutions. See this issue as an example on one where async resolution was taken: w3c/csswg-drafts#10345.

@johanneswilm
Copy link

The CSS WG, for example, have a system for async resolutions. See this issue as an example on one where async resolution was taken: w3c/csswg-drafts#10345.

Right, but that is a large WG. If we were that large, our processes should also change and this could be one good option.

The asynchronous part is that you can protest against a decision after it has been made. In practice, we instead wait with making a decision until every participant is ready to make it (see for example the question of target ranges for EditContext which has been discussed for more than 6 months now as not all participants in that discussion want to take part in the monthly calls).

Also, this is just a template. So if someone's grandmother has died on the day of a call and we get the response from that person in some other way before or after the meeting, it's possible to adjust the PR at that stage with a comment that points out that one engine answered asynchronously, if one thinks that is relevant.

I am not opposed to changing our way of working on getting to decisions. But that should be a topic of discussion, for example at TPAC. It should not come in the form of a PR template that only a small group of people are discussing outside of the meeting.

Feel free to use other wording than what I did as long as the meaning is covered.

@sanketj
Copy link
Member Author

sanketj commented Jun 21, 2024

I am not opposed to changing our way of working on getting to decisions. But that should be a topic of discussion, for example at TPAC. It should not come in the form of a PR template that only a small group of people are discussing outside of the meeting.

Feel free to use other wording than what I did as long as the meaning is covered.

I think we're on the same page that the PR template should just enforce how we already operate as a group, not change it in any way. (We can discuss changes like that separately if needed.)

I interpret implementor interest in this context to be "interest in standardization". That doesn't mean they are interested in implementing the feature in their browser right away - just that when they do implement, they plan to implement the solution in the spec. If at least two implementors are interested in standardization, it makes sense to pursue the feature further in the group, with the goal of having an interoperable and compatible implementation for the interested engines. If just one engine is interested or none, then it doesn't make sense to have a spec for it. That appears to be the way we've been operating (to me at least), and I think this template just makes that more explicit.

Do you see it differently?

@johanneswilm
Copy link

johanneswilm commented Jun 21, 2024

Do you see it differently?

Well yes, I agree that is how we have been working when deciding whether or not to start developing a spec for a specific feature or a major new addition to a spec. And I agree that we should continue to do that. But that is a decision taken after looking at the explainer. Maybe again when looking at whether or not to move to CR, etc. . In the case of the explainer we don't use this template at all. And a PR that makes changes to a spec to move it to CR is just one tiny little change.

The overwhelming majority of PRs are about minor tweaks/adjustments to specs such as "which input type should be used for AI grammar checkers?" At that stage we really want all implementors who have implemented a particular spec to come to agreement and not have to implementors use one inputtype and the third one using a different one. The decision of which input type to use may also be too small to warrant for you to ask the Chromium board to come to a decision that you can then link in the PR and to have everyone else do the same. Or maybe it is a big decision because LLMs are really big right now. You know your organization better than the rest of us. But there are may other changes like that that will not require formal interest statements, etc. simply because very few will care enough.

@sanketj
Copy link
Member Author

sanketj commented Jun 21, 2024

Do you see it differently?

Well yes, I agree that is how we have been working when deciding whether or not to start developing a spec for a specific feature or a major new addition to a spec. And I agree that we should continue to do that. But that is a decision taken after looking at the explainer. Maybe again when looking at whether or not to move to CR, etc. . In the case of the explainer we don't use this template at all. And a PR that makes changes to a spec to move it to CR is just one tiny little change.

The overwhelming majority of PRs are about minor tweaks/adjustments to specs such as "which input type should be used for AI grammar checkers?" At that stage we really want all implementors who have implemented a particular spec to come to agreement and not have to implementors use one inputtype and the third one using a different one. The decision of which input type to use may also be too small to warrant for you to ask the Chromium board to come to a decision that you can then link in the PR and to have everyone else do the same. Or maybe it is a big decision because LLMs are really big right now. You know your organization better than the rest of us. But there are may other changes like that that will not require formal interest statements, etc. simply because very few will care enough.

Ok, so what I'm hearing is that for new features we should make sure there are explicit interest signals from multiple implementors, whereas for normative changes to existing features, we should just file bugs on the browsers that implement the spec in question. Is that accurate?

Draft updates to distinguish implementor commitments for shipped vs. unshipped features.
Additional updates that attempt to encapsulate all aspects of the Editing WG resolution.
Add empty line between checkboxes.
@dandclark
Copy link
Contributor

I tried out the template for #101 and the "For features that have not shipped in all browsers, at least two implementers are interested" part was a little awkward.

I believe we had representatives from both WebKit and Gecko in the meeting where we got the resolution on this, so I'm just linking to the same meeting minutes that I linked to for the "Editing WG resolution on the proposed changes" checkbox. But those minutes don't include an explicit "Yes we support this" from WebKit/Gecko. It doesn't show anyone from WebKit speaking on the issue at all. So the link I'm citing is not really good evidence for checking the box.

Nevertheless given the WG resolution, and the fact that WebKit and Gecko have not implemented EditContext yet anyway, I don't think it's a good use of anyone's time for me to reach out and ask them to leave an affirmative comment on this PR; the WG resolution should be enough.

For the WebKit and Gecko bugs, I just referenced generic "Implement EditContext" bugs.

@sanketj
Copy link
Member Author

sanketj commented Jun 22, 2024

I tried out the template for #101 and the "For features that have not shipped in all browsers, at least two implementers are interested" part was a little awkward.

I believe we had representatives from both WebKit and Gecko in the meeting where we got the resolution on this, so I'm just linking to the same meeting minutes that I linked to for the "Editing WG resolution on the proposed changes" checkbox. But those minutes don't include an explicit "Yes we support this" from WebKit/Gecko. It doesn't show anyone from WebKit speaking on the issue at all. So the link I'm citing is not really good evidence for checking the box.

Nevertheless given the WG resolution, and the fact that WebKit and Gecko have not implemented EditContext yet anyway, I don't think it's a good use of anyone's time for me to reach out and ask them to leave an affirmative comment on this PR; the WG resolution should be enough.

That's good feedback. I think a WG resolution where two or more implementors were present and didn't express an objection to the resolution should be considered enough support for the change to proceed. That was the intent of the content of the template, but I agree it could be captured a bit better.

Thinking about this some more, IMO we should adopt something like what was suggested here: #100 (comment). It'll be a bit wordy, but I think that's the best way to cover both the Editing WG and multiple implementor requirements, without creating the (incorrect) impression of additional processes. I'll draft that up as part of the next iteration. Let me know how that feels.

@sanketj
Copy link
Member Author

sanketj commented Jun 22, 2024

For the WebKit and Gecko bugs, I just referenced generic "Implement EditContext" bugs.

Linking to the broader implementation bug is fine, but I also think it would be fine to say "Not Implementing" (or something to that effect). I don't think it is necessary to file implementation bugs for incremental spec changes if the browser never implemented the feature in the first place.

@saschanaz
Copy link
Member

I guess "not objecting" is fine here for now as this spec is still in early day, but if it gets more progress I think the requirement should ultimately become "active interest", to make sure the WG resolution links to implementation interoperability.

(I.e. I don't think the current patch works outside EditContext and should be adjusted)

@sanketj
Copy link
Member Author

sanketj commented Jun 24, 2024

I guess "not objecting" is fine here for now as this spec is still in early day, but if it gets more progress I think the requirement should ultimately become "active interest", to make sure the WG resolution links to implementation interoperability.

(I.e. I don't think the current patch works outside EditContext and should be adjusted)

The "not objecting" part is how the WG resolves all issues, not just the EditContext ones. When a resolution is proposed, the group is asked to voice any objections. If no objections are shared, the WG takes the resolution. The template is just reflecting that working model.

@saschanaz
Copy link
Member

saschanaz commented Jun 24, 2024

Alright, let's first do the right thing: reflect what the WG currently do. I still think that's not always enough (sadly W3C specs in general have been a pile of unimplemented things even when implementor agreement existed), but I think that doesn't have to be blocker here as this one solves @marcoscaceres' original concern.

@johanneswilm
Copy link

Alright, let's first do the right thing: reflect what the WG currently do. I still think that's not always enough (sadly W3C specs in general have been a pile of unimplemented things even when implementor agreement existed), but I think that doesn't have to be blocker here as this one solves @marcoscaceres' original concern.

I agree that thos can be a real problem in other WG. If in the future we end up with an issue where implementors are not picking up changes to the specs because their representatives did not put an explicit support into the meeting minutes, I agree that we should reconsider the practice.

For now, I don't see this being a problem.

@sanketj
Copy link
Member Author

sanketj commented Jun 24, 2024

@johanneswilm Are you okay with the current wording of the template or did you have additional feedback?

@johanneswilm
Copy link

@sanketj I am fine with it.

@sanketj
Copy link
Member Author

sanketj commented Jun 24, 2024

@whsieh @megangardner Friendly ping. Are you all okay with the format of this PR template?

@sanketj sanketj added the Agenda+ Queue this item for discussion at the next WG meeting label Aug 7, 2024
@whsieh
Copy link

whsieh commented Aug 8, 2024

@whsieh @megangardner Friendly ping. Are you all okay with the format of this PR template?

Seems good to me!

@sanketj sanketj merged commit 365bbe3 into w3c:gh-pages Aug 8, 2024
1 check passed
@sanketj sanketj removed the Agenda+ Queue this item for discussion at the next WG meeting label Aug 8, 2024
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.

5 participants