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

Added issue, pull request templates. #2869

Merged
merged 3 commits into from
Sep 3, 2017
Merged

Conversation

kunagpal
Copy link
Contributor

@kunagpal kunagpal commented Jun 8, 2017

Resolves #2851

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.395% when pulling 58b3cff on kunagpal:issue/2851 into 7647e18 on mochajs:master.

Copy link
Contributor

@ScottFreeCode ScottFreeCode left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a good start; just a few small things that I think should be tightened up here:

  • Change references to "the user('s)" to "you(r)".
  • The Version section isn't particularly reliable without first addressing the possibility that there's a discrepancy between global and local versions of Mocha; something about that (perhaps just a blanket recommendation to uninstall global Mocha?) should go in the prerequisites section I suppose?
  • May as well throw in Node version in the Versions section on top of Mocha version and OS version.

@kunagpal
Copy link
Contributor Author

@ScottFreeCode Ping

Copy link
Contributor

@ScottFreeCode ScottFreeCode left a comment

Choose a reason for hiding this comment

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

Sorry about the delay; just a couple little tweaks to the first set of changes, and then this should be good to go!

* [ ] Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
* [ ] 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
* [ ] Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with:
`node node_modules/mocha/bin/mocha* --version`(Local) and `mocha -v`(Global). We recommend avoiding the use of globally installed Mocha.
Copy link
Contributor

Choose a reason for hiding this comment

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

"node node_modules/mocha/bin/mocha*" -> "node_modules/.bin/mocha" or at least remove the "*".

"mocha -v" -> "mocha --version"

<!--
You can get this information from copy and pasting the output of `mocha --version` from the command line.
Also, please include the following:
* The version and architecture of the operation system being used.
Copy link
Contributor

Choose a reason for hiding this comment

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

"operation system" -> "operating system"

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see something like this:

If applicable, please specify:

  1. The output of mocha --version:
  2. The output of node --version:
  3. Your OS:
  4. Your shell (bash, zsh, PowerShell, cmd, etc):
  5. Your browser and version:
  6. Any third-party Mocha-related modules:

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! I have added some requested changes.

@@ -0,0 +1,45 @@
<!--
Have you read Mocha's Code of Conduct? By filing an Issue, you are expected to comply with it, including treating everyone with respect: https://github.com/mochajs/mocha/blob/master/.github/CODE_OF_CONDUCT.md
For more, check out the Mocha gitter space: https://gitter.im/mochajs/mocha
Copy link
Contributor

Choose a reason for hiding this comment

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

how about instead of "gitter space" "Gitter chat room", since not everyone knows what Gitter is?

Place an `x` between the square brackets on the lines below for every satisified prerequisite.
-->
* [ ] Checked that your issue isn't already filed by cross referencing [issues with the `common mistake` label](https://github.com/mochajs/mocha/issues?utf8=%E2%9C%93&q=is%3Aissue%20label%3Acommon-mistake%20)
* [ ] Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
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 this can be woven into the point below by mentioning transpiled code and environment

<!--
You can get this information from copy and pasting the output of `mocha --version` from the command line.
Also, please include the following:
* The version and architecture of the operation system being used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see something like this:

If applicable, please specify:

  1. The output of mocha --version:
  2. The output of node --version:
  3. Your OS:
  4. Your shell (bash, zsh, PowerShell, cmd, etc):
  5. Your browser and version:
  6. Any third-party Mocha-related modules:

### Requirements

* Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
* All new code requires tests to ensure against regressions
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a period (.)


<!-- Explain what other alternates were considered and why the proposed version was selected -->

### Why Should This Be In Core?
Copy link
Contributor

Choose a reason for hiding this comment

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

inconsistent capitalization


<!--

We must be able to understand the design of your change from this description. If we can't get a good idea of what the code will be doing from the description here, the pull request may be closed at the maintainers' discretion. Keep in mind that the maintainer reviewing this PR may not be familiar with or have worked with the code here recently, so please walk us through the concepts.
Copy link
Contributor

Choose a reason for hiding this comment

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

We must be able to understand the design of your change from this description. Keep in mind that the maintainers and/or community members reviewing this PR may not be familiar with the subsystem. Please be verbose.


### Why Should This Be In Core?

<!-- Explain why this functionality should be in mocha as opposed to a package -->
Copy link
Contributor

Choose a reason for hiding this comment

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

capitalize Mocha

Copy link
Contributor

Choose a reason for hiding this comment

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

"as opposed to its own package"


### Applicable Issues

<!-- Enter any applicable Issues here -->
Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase "issues"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add something like:

Mocha follows semantic versioning: http://semver.org
Is this a breaking change (major release)? Is it an enhancement (minor release)? Is it a bug
fix, or does it not impact production code (patch release)?

@boneskull boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Jun 15, 2017
@ScottFreeCode
Copy link
Contributor

I don't know if I mentioned it before, but I really appreciate how the common-mistake label link brings up both open and closed issues. 👍

@kunagpal
Copy link
Contributor Author

kunagpal commented Jun 21, 2017

@ScottFreeCode Ping

@ScottFreeCode
Copy link
Contributor

@boneskull Did you get a chance to check that your requests were addressed to your satisfaction? Is there still anything that's incorrect or missing, or otherwise might prevent users and contributors from getting us what we need simply by following the template (as opposed to minor cleanup, such as non-code typographical corrections, that we can address later)?

@ScottFreeCode ScottFreeCode added the area: repository tooling concerning ease of contribution label Aug 25, 2017
@boneskull boneskull self-assigned this Sep 3, 2017
@boneskull
Copy link
Contributor

wfm, thanks!

@boneskull boneskull merged commit 5895671 into mochajs:master Sep 3, 2017
@boneskull boneskull removed their assignment Dec 12, 2017
sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
* docs(meta): Added issue, pull request templates. 📜 [ci skip]

* Addressed PR#2869 comments [ci skip]

* Addressed some more comments on PR#2869 [skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: repository tooling concerning ease of contribution semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants