Skip to content

PR template addition #411

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

Merged
merged 2 commits into from
Feb 16, 2018
Merged

PR template addition #411

merged 2 commits into from
Feb 16, 2018

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Feb 14, 2018

We would like to show users how a good pull request look like. I used code highlight for the template, is that fine or better solutions ?

The main points that should be coevred:

  • describe your changes (what is being changed and how)
  • how it was tested (targets, toolchains, etc)
  • pull request type (can be one of, not a mixture!)

The template is being updated here ARMmbed/mbed-os#6078

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

One small nit, but not blocking. LGTM!


Fix problems which could leave deep sleep locked unintentionally, along with adding tests to verify this behavior is fixed.

Tested locally with two targets, all toolchains.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend we be specific here, to encourage others to be specific as well.

Replace "two targets" with something like "K64F and K66F".


# Pull request type

- [x] Fix
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the checklist.

Copy edit file for grammar and complete sentences.
@0xc0170
Copy link
Contributor Author

0xc0170 commented Feb 16, 2018

@adbridge @theotherjimmy @kjbracey-arm Please review

@adbridge adbridge self-requested a review February 16, 2018 10:40
Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

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

Agree with Cruz about being specific as to which two targets were tested against. But otherwise looks like a nice simple example. Can always be refined down line, good to get something in.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Feb 16, 2018

Agree with Cruz about being specific as to which two targets were tested against. But otherwise looks like a nice simple example. Can always be refined down line, good to get something in.

I had it first (named two targets I randomly chose) , but did not want to pick up only two targets (which ones we would select), lets see how this goes with users (what info are we going to get) and can add some info about being more specific

@AnotherButler
Copy link
Contributor

Is it OK to merge this before 6078 merges?

@AnotherButler AnotherButler merged commit 841272a into ARMmbed:5.7 Feb 16, 2018
@cmonr
Copy link
Contributor

cmonr commented Feb 16, 2018

@AnotherButler Yup! It even seems like that PR is waiting on this! ARMmbed/mbed-os#6078 (comment)

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