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

Improved first time contributor DX #43

Open
Keyrxng opened this issue Sep 9, 2024 · 5 comments
Open

Improved first time contributor DX #43

Keyrxng opened this issue Sep 9, 2024 · 5 comments

Comments

@Keyrxng
Copy link
Contributor

Keyrxng commented Sep 9, 2024

Enforcing Org Standards for New Contributors

It has become a regular issue that new contributors are not adhering to conventional commit standards. This leads to repetitive interactions with each pull request (PR), where we educate them about our organization-wide standards.

In other large open-source projects, bots automate initial acknowledgements of standards, such as 'accepting terms and conditions' or signing off on specific practices. I propose a similar automated approach to enforce these standards strictly.

High-Level Enforcement via Bot

  • If CI is not green, and a review_requested event is triggered, the bot will apply a requested_changes review and comment: "CI has failed and is required before requesting a review."

Accommodations for First-Time Contributors

  • For first-time contributors, we can utilize an API to determine if this is the contributor's first task, covering anything from /start to closed_as_complete. Or use a JSON based storage simply for Username:HasAcknowledgedTerms, if the user name is not present we request they accept, if it's present standard /start flow.
  • If a contributor has not closed_as_complete or this is their first open PR, we should:
    • Comment on the issue (preferably) or the PR. This ensures they are aware of the guidelines upfront and do not need to backtrack and amend commits.
    • Clearly state our high-level contributing guidelines in the comment. Currently, we lack an official contributing.md, but we enforce several unwritten rules such as:
      • Use yarn
      • Ensure usage of version v1.22
      • Follow conventional commits
      • Always use the formatting scripts provided

Acknowledgement Requirement

  • Implement a system where contributors must "accept" an acknowledgement of these standards before we assign the issue to them. This process:
    • Saves reviewers time by reducing the need to repeatedly educate on org-wide standards.
    • Improves Developer Experience (DX) as contributors are informed upfront, preventing the need to backtrack significantly on their PRs.
    • Enhances professionalism and presents the project as more official.
    • Helps in filtering out contributors who may find these standards too demanding or complex, ensuring a better fit for the project's needs.

Benefits

  • Time Efficiency: Streamlines the review process by setting clear expectations from the start.
  • Improved DX: Reduces the burden on new contributors by providing clear guidelines upfront.
  • Professional Image: Enhances the project's professionalism and operational efficiency.
  • Quality Control: Helps maintain a high standard of contributions by ensuring all contributors adhere to set practices.

This plugin or a new one?

A dedicated plugin would be great actually as we could allow for a lot more customization of the terms and standards etc by allowing them to pass in their own custom markdown for tables etc as opposed to restricted to using our pretty /start message. Decouples from /start more but obv still coupled with it slightly. Allows for simpler per-repo customization as you do not have to consider the /start setup also.

This plugin, really coupled together. The config will become big and messy probably and it adds a whole new branch of functionality to this worker.

here is an example

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 9, 2024

@0x4007 @rndquu @gentlementlegen @whilefoo rfc

@0x4007
Copy link
Member

0x4007 commented Sep 9, 2024

Always decouple if possible. We can consider the following actions:

  1. Close pull
  2. Convert to draft
  3. Request changes review state

I think close pull is the strongest signal that they need to redo it because they didn't follow conventional commits but at the same time it seems pretty drastic.

Convert to draft means they will need to force push or manually amend the previous commit message which is tedious.

Request changes will negatively affect their XP once that is implemented. This might be the best actually, but then they still have to manually amend or force push again which is tedious.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 9, 2024

I think close pull is the strongest signal that they need to redo it because they didn't follow conventional commits but at the same time it seems pretty drastic.

Close pull is too heavy handed I think as if we use all CI rather than just conventional commits, the PR would be closed for failed tests for example.

manually amend the previous commit message which is tedious... force push again which is tedious.

We simply cannot prevent this if they open a PR first before being informed. I feel it's unfair to apply negative effects to a contributor if they aren't made aware of it upfront.

However, I think that if they are informed upfront via the issue then yeah negative effects all day long as they didn't listen.

But I know the problem with that is notifications but we can limit that by having the "accept" action be a simple reaction on the bot comment or the toggling of a tasklist item (I think we can catch them via api). Once accepted the /start command can simply edit the acknowledgement comment and assign them as normal. We do not get a notification for an edit but the contributor would if tagged although they'd be looking at the issue so probs not needed.

I hadn't considered negative xp, but I think that sounds completely appropriate. Perhaps we allow workflows to be defined which should be omitted from this check via a config item too that's probably a good to have


also what should we call this new plugin?

@0x4007
Copy link
Member

0x4007 commented Sep 10, 2024

Close pull is too heavy handed I think as if we use all CI rather than just conventional commits, the PR would be closed for failed tests for example.

If its a draft, as per our instructions, we can let it slide. This is relatable for myself: I generally will open a draft and it could be pretty messed up. After checking the pull request diff view, and seeing CI, I'll usually clean things up. I think this is acceptable.

We do not get a notification for an edit

Pretty sure we do, as well as for emoji reactions.

also what should we call this new plugin?

We can always rename

@rndquu
Copy link
Member

rndquu commented Dec 9, 2024

We've already had this feature of a "first time contributor" welcome message. We could add a new parameter welcomeMessage which accepts a markdown. Then on any /start check that if this is a new contributor then display welcomeMessage with all the contribution guidelines.

Forcing contributors to accept the terms seems to be an unnecessary step which complicates the UX.

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

No branches or pull requests

3 participants