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

Add contribution templates + minor doc improvements #112

Merged
merged 2 commits into from
Nov 8, 2018
Merged

Conversation

Ma27
Copy link
Contributor

@Ma27 Ma27 commented Nov 4, 2018

This is just a proposal, I think it's helpful for first-time contributors to provide them a template which "tells" them what kind of information is needed.

@codecov
Copy link

codecov bot commented Nov 4, 2018

Codecov Report

Merging #112 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #112   +/-   ##
=======================================
  Coverage       100%   100%           
  Complexity      133    133           
=======================================
  Files             6      6           
  Lines           556    556           
=======================================
  Hits            556    556

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66e9244...05e641d. Read the comment docs.

Copy link
Member

@calbrecht calbrecht left a comment

Choose a reason for hiding this comment

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

What are your thoughts about the following (may be adapted to PR template), extra diamond plated, branding version?
[ ] Bug in MO4 sniffer rule.
[ ] Bug in MO4 auto-fixer.
[ ] Bug in MO4 documentation.
[ ] Request to extend MO4 sniffer with specific rule.
[ ] Request to extend MO4 auto-fixer for specific rule.
[ ] Request to extend MO4 documentation.
[ ] Other (explain):

@Ma27
Copy link
Contributor Author

Ma27 commented Nov 4, 2018

fixed, train wifi seems to work today %)


* [ ] Bugfix
* [ ] New Feature
* [ ] Breaking change
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "Breaking Change" should be on a separate Line/Section and the third point should be "something else" or "other".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the addition of "else": full ack 👍

Regarding the "Breaking change" box: how would you propose to name this section?

I certainly get your point, I wasn't sure about this either, but in the end I thought that nobody forbids you to check multiple boxes (these are intentionally check boxes, not radio boxes). But as I said, I'm not sure about this and happy to discuss :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, maybe just a headline "Is this a breaking chnage"? Otherwise we could also handle this with setting a respective label when reviewing.

@Ma27
Copy link
Contributor Author

Ma27 commented Nov 8, 2018

@mmoll I slightly altered the PR and placed the breaking change part into its own section. Would you mind having another look? :)

@mmoll mmoll merged commit 408f293 into master Nov 8, 2018
@mmoll
Copy link
Contributor

mmoll commented Nov 8, 2018

merged, danke @Ma27!

@mmoll mmoll deleted the doc-improvements branch November 8, 2018 12:29
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.

None yet

3 participants