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

make PR type box list "fancier" without making it a github tasklist #6669

Merged
merged 1 commit into from Apr 23, 2018

Conversation

Projects
None yet
5 participants
@ithinuel
Member

ithinuel commented Apr 18, 2018

Description

This basically make the default PR template looks like this PR.

Pull request type

[ ] Fix  
[x] Refactor  
[ ] New target  
[ ] Feature  
[ ] Breaking change

@0xc0170 0xc0170 requested review from cmonr and adbridge Apr 18, 2018

@0xc0170

Not certain about the note, is it necessary ? A user should not change the format. Why we do not have - there should be in git history (we changed it to the format its now)

@adbridge adbridge requested review from adbridge and removed request for adbridge Apr 18, 2018

@adbridge

I'm not sure this is actually adding anything. Use of a 'X' or 'x' is probably hard to mandate and any automated checking we apply will allow for both.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 18, 2018

Also I don't think the note is necessary. We already have a comment saying do not change the layout.

@ithinuel

This comment has been minimized.

Member

ithinuel commented Apr 18, 2018

Everytime I create a PR and see these boxes I'm tempted to create the kind of box list I know.
The choice of not using them it not obvious (to me) at a first glance.

@adbridge It's not much about the x or X but on the monospaced box list or markdown task list.

[X] like
[ ] that

or

  • like
  • that

Writing the later one would make the PR have a "todo" list displayed that we do not want.

@ithinuel ithinuel force-pushed the ithinuel:make_pr_template_fancier branch from 7d60afc to 7828c0f Apr 18, 2018

@ithinuel

This comment has been minimized.

Member

ithinuel commented Apr 18, 2018

@adbridge I removed the note move the explanation closer to where we ask not to change the layout.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 18, 2018

@ithinuel Either way to get either of the changes you mention would mean somebody has changed the layout ..... Which we already tell them not to do. We 'could' update the note to say something like.
"Please note this is not a GitHub task list, indenting the boxes or changing the format to add a '.' or '*' in front of them would change the meaning incorrectly. The only changes to be made to this PR header are to add a description text under the description heading and to add a 'x' to the correct box."

@ithinuel ithinuel force-pushed the ithinuel:make_pr_template_fancier branch from 7828c0f to 345bc49 Apr 18, 2018

@ithinuel

This comment has been minimized.

Member

ithinuel commented Apr 18, 2018

Would this last update be Ok ?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 18, 2018

LGTM :)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 19, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 19, 2018

Build : SUCCESS

Build number : 1795
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6669/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 19, 2018

@kjbracey-arm @mikaleppanen Can you review the latest test result (related to echo test). We noticed this failure, seldom in the last days.

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 20, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 20, 2018

@kjbracey-arm @mikaleppanen Can you review the latest test result (related to echo test). We noticed this failure, seldom in the last days.

Not networking , I missed the driver test in there. Never-mind, drivers /host issue.

@cmonr cmonr merged commit f7a707a into ARMmbed:master Apr 23, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9626 cycles (+1073 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@ithinuel ithinuel deleted the ithinuel:make_pr_template_fancier branch Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment