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

Best Practices #1474

Merged
merged 10 commits into from
Jun 21, 2024
Merged

Best Practices #1474

merged 10 commits into from
Jun 21, 2024

Conversation

jafeltra
Copy link
Collaborator

Description: This PR documents best practices that development for SUSHI should follow. It also adds a basic PR template to encourage detailed PR descriptions. I used it here just to give you a feel for what it would look like. If we don't like the PR template, I can tweak it or remove it.

Testing Instructions: Review the new Best Practices document and the PR template, and let me know if there are any best practices you like that I haven't included or if I've been too strict in any areas.

Related Issue: N/A

- Includes a simple PR template
@KaelynJefferson
Copy link
Collaborator

Looks good! Maybe one change is to change "Follow the pull request template..." to "Follow this pull request template... :" just for even more clarity that that is the template, and it's not located somewhere else?

@jafeltra
Copy link
Collaborator Author

Maybe one change is to change "Follow the pull request template..." to "Follow this pull request template... :" just for even more clarity that that is the template, and it's not located somewhere else?

As long as we end up keeping the pull_request_template.md file I added, when you create a new PR, the description should be automatically populated with the template. That's why I had just said "follow the pull request template" since I thought it would be clear what the template is when you create the PR. But since it sounded ambiguous to you, maybe it would be better to just remove the reference to a template and just say something like "Create a detailed PR description that contains the following information:..." That would cover the PR template if we keep that and provides all the info even if we don't keep it. How do you feel about that update?

Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

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

This looks good! I only found one thing that needs to be changed.

- SUSHI uses [Prettier](https://prettier.io/) for code formating.
- To run the prettier on all code, run `npm run prettier`. Ensure there are no issues reported.
- Ensure any new dependencies do not contain any known security vulnerabilities
- To check for known security vulnerabilities, we recommend using [npm-audit](https://fshschool.org/). Run `npm audit` and ensure there are no new issues on your branch.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably isn't supposed to link to FSH School.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wow, you're very right! Thanks for catching that!

Fixed in d140715.

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

This is looking good. I've made a few suggestions to consider.

.github/pull_request_template.md Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Show resolved Hide resolved
- Add info for test coverage command
- Add info for prettier and lint fix commands
- Add info about ensuring new dependencies are up to date
- Require use of squash and merge
@jafeltra
Copy link
Collaborator Author

@mint-thompson and @cmoesel -- thanks for the feedback! I believe everything mentioned so far has been addressed.

Copy link
Member

@cmoesel cmoesel 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 the extra details are helpful. I have a few more nitpicky suggestions.

BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
BEST-PRACTICES.md Outdated Show resolved Hide resolved
cmoesel
cmoesel previously approved these changes Jun 19, 2024
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Alright! This looks good!

- Recommend use of Zulip for discussion
- Note that if the conversation concludes that changes to the tooling
  are required, a GitHub issues should be created
CONTRIBUTING.md Outdated Show resolved Hide resolved
@jafeltra jafeltra marked this pull request as ready for review June 21, 2024 16:27
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

HL7 has reviewed and approved. We're ready to merge!

@cmoesel cmoesel merged commit b8bfc95 into master Jun 21, 2024
14 checks passed
@cmoesel cmoesel deleted the best-practices branch June 21, 2024 17:35
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.

4 participants