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

Additional guidance for contributions #381

Merged
merged 4 commits into from
Apr 19, 2023
Merged

Conversation

wchao1115
Copy link
Collaborator

Following the WG teleconference call on 4/13 on the topic of contributions guidelines, I've added more text into our existing contributing.md file to further clarify the types of change and provide additional guidance on how to work on a PR and what to expect from the working group's review process. Most of this change is what I verbally outlined on the call that day with additional clarification. Further feedback here is welcome.

BTW, I of course realize that this PR itself doesn't follow the guidelines it conveys in a strict sense, but the fact that this PR does follow an agreement from our WG conference should give it a pass. ^^

Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

@wchao1115 much thanks for the PR!

I believe this PR, and specifically its guidelines for new content, will fix our long-standing issue #231 :-)

## Goal
Members of the Working Group contribute their work and time on a volunteering basis. Our goal is to provide a process that expedites proposing and reviewing changes to the specification within the scope of the [charter](https://www.w3.org/2023/04/web-machine-learning-charter.html) that results in clear and consistent documentation while reducing the opportunities for accidental mistakes and misinterpretation.

## Type of change
Copy link
Member

Choose a reason for hiding this comment

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

I personally like the category names introduced in this PR because they're more generic and as such familiar to a broader group of OSS contributors. We could consider mapping these to the W3C Process defined classes of changes that are more W3C specific and introduce both the W3C Process and Patent Policy linkage. Another possible W3C Process alignment is to speak about substantive change rather than material impact. Established W3C terminology is to speak of normative vs. informative or non-normative content (W3C Process does not have formal definitions for these terms so the generic meaning is assumed).

Using W3C parlance in this guideline doc might be too much so I'm not pushing that strongly. @dontcallmedom WDYT?

CONTRIBUTING.md Outdated
## Process details
Wording change does not require opening a GitHub Issue, as the change in the pull request is self-sufficient. However, wording changes should improve the overall readability and interpretation of the content in an obvious way. Keep in mind that these are subjective qualities, and not everyone may agree to the change. Ultimately, it is up to the consideration of editors whether to accept it.

Similarly, a stylistic change does not necessarily require opening a GitHub Issue. It does, however, require buy-ins from the Working Group in order to proceed. The best way to propose this type of change is to attend one of the bi-weekly Web Machine-Learning Working Group teleconference calls and get a vote. A practical way to reach out to the Working Group to get invited to the teleconference call is to post a GitHub Issue giving a rough explanation of the proposed change and ask to be invited.
Copy link
Member

Choose a reason for hiding this comment

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

s/Machine-Learning/Machine Learning

I'd drop "and get a vote" because deciding by voting has a special meaning in W3C and we're not doing that for technical work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good to know. Will fix that.

Bug fixes and new content changes should proceed as follows:
1. **Open an issue in GitHub Issues** with a brief description of the problem and a potential solution if it's not already obvious. A proposal or suggestion for improvement may need a bit more explanation with possible references to related information. An active issue is the best way to get attention. Members of the Working Group scan active issues constantly.
2. **Prepare the change in a pull request** and put a reference to the active issue(s) the change is addressing in the description. We prefer that a pull request is represented by a single type of change as outlined in the previous section for a speedy review and approval. Conversely, a specific change should also be captured by a single and not multiple pull requests. This helps to reduce the dependency between pull requests and the chance for the specification to be left in a transient state between multiple pull requests. Exceptions to this should be discussed and approved by the Working Group in one of our bi-weekly calls.
3. **Close the issue** once the pull request is reviewed and merged. Make sure to resolve any error that arises during the merge and check the post-merged published result. The Bikeshed document format isn't very good for an automatic merge, you may need to intervene and manually correct the merge's mistakes if any. You also want to make sure all the GitHub Actions that are put in place to catch document issues are all clean before merging the change into the main branch.
Copy link
Member

Choose a reason for hiding this comment

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

We may want to add (not necessarily in these exact words):

A pull request can be merged when it has received adequate review from the Working Group. The editors and the chair are responsible for ensuring review is sought from appropriate people. For substantive changes, adequate review is two or more reviewers. The bi-weekly teleconference calls provide another venue for providing feedback and review open pull requests.

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM. This is a great guideline. Thanks much @wchao1115 !

@zolkis
Copy link
Collaborator

zolkis commented Apr 19, 2023

From the description it is not clear whether e.g. adding a missing algorithm is new content or wording change.
Actually, much of the PRs for adding an algorithm will contain new content, wording change and might contain stylistic adaptations as well.
A long-term learning over my career has been that classifying the world does not necessarily help with simplicity, since categories will always be diffuse. One should seek classification of operations instead. For instance, my PRs will actually be "adding algorithm to op", and that is more clear/simple than saying "a new PR that contains new content, wording change and stylistic changes", or, splitting it into 3 PRs.

Anyway, this is a good/clear start, and practice/experience may refine it if/where needed.

@zolkis
Copy link
Collaborator

zolkis commented Apr 19, 2023

Another question emerges: how a PR is supposed to be flagged for one or more of the categories?
Currently labeling is restricted, so the only possibility is to mention it in words in the PR itself.

zolkis pushed a commit to zolkis/webnn that referenced this pull request May 15, 2023
* Draft

* More text.

* mote text

* Address PR feedbacks.
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.

5 participants