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

[Optional] Add Danger to our development workflow #75

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

pengyin-shan
Copy link
Collaborator

@pengyin-shan pengyin-shan commented Mar 22, 2023

Danger library [#70]

This is an optional step targeting to enhance our coding environment, we can discuss if we want to add this extra check or not

Danger (https://github.com/danger/danger) is a tool to automate some team conventions on code review, such as setting up reminders to update the CHANGELOG, adding more PR descriptions, adding tests for newly implemented methods, etc. It targets to give suggestions, so we can approve PRs even Danger test is not passed. It is totally based on our needs.

My previous team used it to prevent programmers from forgetting some contribution rules, such as changelog updates. I borrowed the rules but we are totally open to changing whatever the best for us. The code to define the rules is in dangerfile.js.

So far, the rules of danger suggestions are:

  1. Add changelog
  2. If the programmer feels that no need to update the CHANGELOG, add #trival to PR to eliminate this danger check
  3. Encourage us to write at least some descriptions (line>=3) in PR so that we can refer back to it
  4. Big PR (>500 lines of change) should be separated into smaller PR
  5. Any working in process PR should have [WIP] somewhere so that code reviewers don't need to check
  6. (Disabled currently since we don't have unit test) if someone makes lots of changes but with no test, Danger will give a reminder
  7. (Update on Apr 14, 2023) If anyone modifies the .eslintrc file, GitHub will show a warning in PR to remind the submitter to double-check the necessity of modifying this file.

If we proceed with this PR, we will have a new Github action that runs whenever there's a push operation. We could get something like:
image

@pengyin-shan pengyin-shan requested a review from navarroc March 22, 2023 17:09
@pengyin-shan pengyin-shan requested review from ylyangtw and removed request for ywkim312 and ziyue5 January 10, 2025 22:29
@pengyin-shan pengyin-shan self-assigned this Jan 10, 2025
@pengyin-shan
Copy link
Collaborator Author

Hi @navarroc @sandeep-ps @ylyangtw , this is an old PR that will add danger to the GitHub workflow to automate some PR review points and is ready for your review.

Copy link
Contributor

@ylyangtw ylyangtw left a comment

Choose a reason for hiding this comment

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

Looks good to me. Approve

CHANGELOG.md Outdated
@@ -272,6 +273,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
### Added
- Popup alert for unfinished programs [#58](https://github.com/policy-design-lab/pdl-frontend/issues/58)
- Navigation from PDL title and PDL logo [#51](https://github.com/policy-design-lab/pdl-frontend/issues/51)

Copy link
Member

Choose a reason for hiding this comment

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

Changes in lines 276-278 could be removed as they are not part of this PR. Thanks.

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 catch, should be resolved in my last commit

push:
branches:
- main
- develop
Copy link
Member

Choose a reason for hiding this comment

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

Should this not run on PR since it'll be easier to catch any missing entries before the PR is merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean. Looking back, I think it makes more sense to use:

on:
  pull_request:
    branches:
      - develop

This way, it will help to enhance PR creation practices without interrupting current release practices. Do you think will work better?

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.

3 participants