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

Maintainer docs on "good first" and "help wanted" issues #3889

Merged
merged 14 commits into from
Apr 8, 2024
Merged

Conversation

zackkrida
Copy link
Member

Fixes

Fixes #2340 by @sarayourfriend

Description

Adds maintainer-focused documentation with advice and predef responses for new contributor issues.

Testing Instructions

Read the document and make sure it makes sense. Please also check any md blocks intended to be copy/pastable replies and make sure they're formatted correctly and will work in the GitHub UI.

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@openverse-bot openverse-bot added 🟧 priority: high Stalls work on the project or its dependents 🌟 goal: addition Addition of new feature 📄 aspect: text Concerns the textual material in the repository 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Mar 8, 2024
@github-actions github-actions bot added the 🧱 stack: documentation Related to Sphinx documentation label Mar 8, 2024
@zackkrida
Copy link
Member Author

@sarayourfriend I intend to finish this next week; if you happen to be on GitHub any point prior to its completion I'd love an early review on the general direction and approach taken by the document. Thanks!

Copy link

github-actions bot commented Mar 8, 2024

Full-stack documentation: https://docs.openverse.org/_preview/3889

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

New files ➕:

Changed files 🔄:

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

Love the overall approach so far! Left some suggestions for things to consider adding or expanding on. The response scripts could be pretty helpful especially in PR reviews for things like linting failures.

@obulat obulat removed the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Mar 11, 2024
zackkrida and others added 6 commits March 13, 2024 13:55
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
@zackkrida zackkrida changed the title WIP: Maintainer docs on "good first" and "help wanted" issues Maintainer docs on "good first" and "help wanted" issues Mar 21, 2024
@zackkrida zackkrida marked this pull request as ready for review March 21, 2024 18:03
@zackkrida zackkrida requested a review from a team as a code owner March 21, 2024 18:03
@zackkrida
Copy link
Member Author

I'm not quite certain this is fully ready to ship, but I've been looking at it for a long time and could use an injection of fresh ideas and suggestions. I find myself struggling with the breadth of the document and if it should be a full-on "managing community contributions" guide or if it should remain under the lens of "good first issues". There is significant overlap there anyway but again, I could use some help!

@zackkrida
Copy link
Member Author

Ah, re-reading the issue description has given me some inspiration for more content:

Boilerplate text for "how do I get help working on this issue"
- How to identify a "good first issue", "help wanted", no label, and "staff only" issue
- How to pick the technology labels for "good first issue" and "help wanted" issues
- Tips for sufficiently describing the change required to close the issue keeping in mind the label context (good first issue vs help wanted, etc)
- Include prompts to record or link to Openverse domain-specific documentation if relevant
- Include prompts to link to relevant pieces of code or related PRs as examples
- Include prompts to link to external documentation related to the technology labels if appropriate

I will make more updates tomorrow but would still value feedback now!

Copy link
Collaborator

@sarayourfriend sarayourfriend 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 this is in a good enough state to merge, and we can iterate on as we like. I'll wait to approve until you've shared that you're done adding/changing the documents, but the suggestions I've left are improvements, rather than blockers, and are up for discussion in any case.

Please check out our
[welcome](https://docs.openverse.org/general/contributing.html) and
[quickstart](https://docs.openverse.org/general/quickstart.html) documentation
pages for getting started with setting up your local environment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might also be good to include a prompt for maintainers to share the particular "apps" quickstarts the contributor will need to keep in mind. Something like "Pay particular attention to the quickstarts linked for API, as that is where the changes need to be implemented, and you need to run the API locally to verify your changes".

Comment on lines 89 to 92
It is recommended that first-time contributors begin with a "good first issue".
For more information on how Openverse maintainers think about "good first
issues", please see
[our additional documentation](/meta/contribution/good_first_and_help_wanted_issues.md).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The audience of the linked document is a bit mixed, if we include it here. Is it for maintainers or for contributors? Moving the definition of a good first issue to a separate document from the one that covers how maintainers help contributors complete their early contributions (predefs, meta-thoughts about good/bad Openverse domains for good-first-issues, etc).

Some version of the "What makes an issue a good first issue?" section makes sense to have as an all-audience context. How to write the good first issue, on the other hand, is just for maintainers. A longer document might discourage contributors looking for something quick from reading it, potentially having them miss out on the context.

As in the original discussion, I think NeoMutt's separation makes a lot of sense, and has layers to "ease" someone into understanding the different levels of possible contribution.

Here's the definition of "Easy Coding Tasks", linked from the "Newbie Tutorial". Ignoring the list of specific tasks in that definition, the definition itself is really straightforward, and encapsulates the philosophy behind it. It's good to have a deeper explanation for maintainers to wrap their heads around, especially because we're also investing time in writing good descriptions for these tasks. For contributors, however, a higher-level description of the expectations they should have as coder and that we will have as reviewer/maintainer, would be faster to incorporate.

All of this is just to share an example for why I think separating the definition of "good first issue" for a primary audience of the contributor, rather than linking to the maintainer oriented definition, is important. I hope the example shows an effective way to layer this. I don't know if NeoMutt have deeper documentation elsewhere for their maintainers regarding this, but their docs are significantly more concise for new contributors than ours are to begin with. I think that's a good model, because if it's too long or looks too involved, people won't read it and fewer will be better off.

At least these three points (we don't have the mentorship ability) are true for us, and say things in a language a contributor would understand quickly:

Simple – You don’t need to be a C master to do them
Understandable – You don’t need to know the insides of NeoMutt
Low priority – If it takes you a while, it doesn’t matter

For Openverse, maybe:

Simple - You don't need to be a JavaScript or Python master to do them
Understandable - Expectations are clearly documented in the issue and you don't need to learn everything about Openverse to complete them
Low priority - If it takes you a while, it doesn't matter

I can't think of a better way to describe low priority 🙂

Another caveat for maintainers: good first issue shouldn't be part of a project stream at all, probably! Maybe only very small bugs or quality-of-life improvements. Unfortunately, we have very little feature related work that would be "good first issue" under that definition, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also realising here that NeoMutt explicitly says C master, implying, you need to know something about C to begin with. I wonder if we need to set that kind of bar as well. Like for each app, we would want to say something like:

  • You should be comfortable with Python/TypeScript
  • You should be familiar with the basic concepts for Django/Vue/Airflow
  • etc.

Is the expectation that a good first issue is a way to learn about the technologies, or about Openverse? That's probably a good thing to clarify: what is our intention for someone completing good first issues? If we intend contributions to be focused on both, is there a baseline expectation for where someone is? Do we expect to need to leave feedback on a PR regarding the basics of JavaScript/Python (is vs == in Python, == vs === in JS, for example), or should contributors, even first time contributors, already have that late-beginner/early-intermediate level of understanding about those languages (acknowledging there are sometimes complex edge cases that even advanced users of those languages struggle with)?

This is a bit of target-audience analysis that our issues could benefit from, and help narrow the scope of what details we need to include in a good first issue for them to be considered complete and appropriately detailed for the level of experience we anticipate and hope new Openverse contributors bring.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I am going to remove this reference from this file. Your comments are fantastic so I'll make sure to reference them in a follow-up issue on documenting good first issues for contributors rather than maintainers.

Concerning this question:

Is the expectation that a good first issue is a way to learn about the technologies, or about Openverse?

I think the later, primarily learning about Openverse. Refining one's understanding of different technologies can be an aspect of writing any code, but I do not think it is an explicit goal we need to promote. Ideally, issues are documented clearly enough that contributors can gauge if their current skillset is sufficient to cover the issue, or if they're nearly there and can fill any gaps on-the-fly.

Do we expect to need to leave feedback on a PR regarding the basics of JavaScript/Python (is vs == in Python, == vs === in JS, for example), or should contributors, even first time contributors, already have that late-beginner/early-intermediate level of understanding about those languages (acknowledging there are sometimes complex edge cases that even advanced users of those languages struggle with)?

IMO our contributors should have this level of late-beginner/early-intermediate knowledge. If they do not it's likely a PR will have significant issues and either be closed or taken over by a maintainer. I don't think we should spend time walking people through these types of learnings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, a separate issue makes sense. I share your opinion on what appropriate expectations would be for our issues, and in particular think it aligns well with the time and focus we are able to dedicate and explicitly commit to for any PR review or contributions in general.


| Scenario | Recommended Response Time (Days) |
| -------------------------------------------- | ---------------------------------------------------------------- |
| New PR submitted | Typically priority based, but for community PRs should be 3 days |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we update the PR review reminder bot to check the PR author, and use a 3 day expectation for anyone outside the maintainers list?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a great idea! Probably best to do it in this PR rather than follow-up, so I'll try and add that asap.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, given my AFK, I'll make a separate issue for this. It requires adding a new method to our GitHubApi class in the catalog and making small but non-trivial changes to the pr review reminder dag that I unfortunately don't have time for.

@zackkrida zackkrida marked this pull request as draft March 25, 2024 13:21
@zackkrida
Copy link
Member Author

Thanks, @sarayourfriend! I'm going to focus on two things:

  1. Making minimal changes to get this PR shippable
  2. Organizing feedback into a new issue for follow-up documentation

@zackkrida zackkrida marked this pull request as ready for review March 31, 2024 15:37
@zackkrida
Copy link
Member Author

This is ready for review. I've constrained the scope to be a good initial document directed at maintainers. I will document two follow-up issues to:

  • Add a small, contributor-focused document or section explaining our Good First Issues
  • Update the PR Review Reminder DAG to check if the PR author is not a maintainer and set the urgency to 3 days.

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

All my feedback has been address, but I'll refrain from putting "approve" on this, because I think it's a good thing for as many maintainers to read and think about as possible, and I was the one who drove this proposal in the first place, and do not want to over-influence the approach and ideas we agree to.

@openverse-bot
Copy link
Collaborator

Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR:

@AetherUnbound
@dhruvkb
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend1 days, this PR was ready for review 3 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2.

@zackkrida, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

  1. Specifically, Saturday and Sunday.

  2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range.

@AetherUnbound AetherUnbound removed their request for review April 4, 2024 00:49
Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

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

LGTM!

I would like to see future iterations to it add more scenarios for predefs but this addresses the most common scenarios and the guidelines for good issues are totally in line with what I would expect from a different open-source project were I to participate as a new contributor.

Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

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

Thank you so much for compiling these documents! It will be really helpful.

links and a summary of the important aspects when documentation is detailed
- Ideally "good first issue"s require essentially no Openverse-specific domain
knowledge, but if they at all do (for example, to explain why a non-obvious
solution is requested) then it must be included
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think adding these headings to all of our issue templates would make the templates too annoying? We could comment out the headings so that if the person writing the issue does not need them, they can simply leave the unnecessary parts as comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, also, I think we should add "Please comment below if you want to work on this issue" to the bottom of the description. Otherwise, if a contributor has never interacted with the issue, we cannot assign it to them.

- Does my PR pass linting? (Test either via precommit or by running `just lint`
manually)
- Do all the tests still pass?
- If JavaScript changes, run `pnpm -r run test`
Copy link
Contributor

Choose a reason for hiding this comment

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

just frontend/run test should work here (for consistency with the other commands)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem with just frontend/run test is that it only runs frontend tests. pnpm -r run test runs test for all workspaces, which will include the eslint plugin, which we've had contributors open PRs for. The test suites run parallel to each other, so it doesn't add any extra time, and prevents any chance of someone getting confused about where the changes they made are relevant. Especially important if we have anything in packages in the future that is depended on by the frontend and not explicitly tested in the frontend suite (which makes sense, we wouldn't test dependencies normally). Using pnpm -r run test here (and ideally everywhere) prevents it from being overlooked in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for explaining!

@zackkrida zackkrida merged commit 0469df5 into main Apr 8, 2024
38 checks passed
@zackkrida zackkrida deleted the fix-#2340 branch April 8, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 aspect: text Concerns the textual material in the repository 🌟 goal: addition Addition of new feature 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: documentation Related to Sphinx documentation
Projects
Archived in project
5 participants