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

chore: add targeted github issue templates #2356

Merged
merged 6 commits into from
Aug 6, 2018
Merged

Conversation

shellscape
Copy link
Contributor

This PR adds targeted issue templates to the repository. Issue template types have been rolled out by github in May (https://blog.github.com/2018-05-02-issue-template-improvements/) and provide a better experience for users, by directing them to a tailored issue template for a particular kind of issue.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Nice! I love the sassy tone. I would suggest to shorten some of the templates, though, and rework the "bug" template a little to promote REPL links and repos as the best possible issue material a little more. Maybe you got some ideas for the wording here?

* Node Version:
* NPM Version:
* Rollup Version:
* ${package} Version:
Copy link
Member

Choose a reason for hiding this comment

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

I do not think these make sense for features? Especially since people should not "skip or remove parts of this template". If their feature is related to a specific environment, I am sure people will tell us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair.

```js
// rollup.config.js

// If your Rollup config is meaty 🍖 and over 20 lines long, please provide a
Copy link
Member

Choose a reason for hiding this comment

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

Rollup configs usually do not tend to get overly large but having a repo is nice in any case :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to reformatting this. I did see a bunch that were sizable when lots of plugins were added, so went with the better safe than sorry approach. If it's not something that happens frequently though, it probably doesn't have to be mentioned.

- Operating System:
- Node Version:
- NPM Version:
- Rollup Version:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put the Rollup version to the top as it is the most important piece here. As Rollup could theoretically be run in a browser, we might prefix the rest with an if applicable:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

### Actual Behavior


### Code
Copy link
Member

Choose a reason for hiding this comment

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

I think this section should be reworked a little to promote the ways of reporting an error that help us the most.

  1. If the issue has to do with faulty code (many issues do) and can be replicated completely in the REPL, then providing a REPL link is usually the best option. A REPL link can easily be checked by any of us with minimal effort and quickly allows us to judge the severity of an issue.
  2. If it cannot be replicated in the REPL, then a repo that exposes the issue is definitely the best option and this should be communicated. There are many situations where people post their rollup configs and hope we figure out what is wrong while the actual problem is caused by something in their code—which we do not have access to.
  3. Only if the options above are not possible—sometimes code cannot be made public—people should post a rollup config and code samples or an incomplete gist. These kinds of issues are the hardest to track down and tend to lie around for some time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. That approach seems much more streamlined and offloads more of the burden of proof onto the user up front. I'll poke at this.

// additional code. remove this block if you don't need it
```

### How Do We Reproduce?
Copy link
Member

Choose a reason for hiding this comment

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

Only necessary if we do not have a REPL link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved this to the top so it's the priority, and added commented out notes for how to provide a minimal reproduction, which are very similar to the template that exists on master. It'll give users a choice for how to provide a repro, and a space for elaborating if needs be.

⚡️ katchow! We 💛 issues.

Please - do not - remove this template.
Please - do not - skip or remove parts of this template.
Copy link
Member

Choose a reason for hiding this comment

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

If we leave this line here, then it should be made clear which parts can be skipped if they do not apply. See below.

### Feature Proposal


### Feature Use Case
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Use case before proposal?

* Node Version:
* NPM Version:
* Rollup Version:
* ${package} Version:
Copy link
Member

Choose a reason for hiding this comment

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

I think the same applies here as with features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye


Hey there! If you need help or tech support then this is not the place to
ask. Please head to [the Rollup Gitter](https://gitter.im/rollup/rollup)
instead or post a question to https://stackoverflow.com/questions/tagged/rollupjs.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could add something like: If you think Rollup's documentation is unclear, insufficient or wrong, consider creating an issue for the documentation instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, nearly verbatim.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Awesome to see these being refined. Have provided a basic review here.

* ask for help on https://gitter.im/rollup/rollup
You're seeing this because you felt none of the other options fit the type of
issue you'd like to create. Please use this opportunity to tell us about your
the type of issue you were looking for, so we can try to accomodate similar
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "about your the type of issue"

Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling: "accommodate"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye

- Rollup Version (if applicable):
- Operating System (or Browser):
- Node Version:
- NPM Version:
Copy link
Contributor

Choose a reason for hiding this comment

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

Rollup has no runtime dependencies so we could remove the npm version, unless they are having TypeScript typing issuese.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's reasonable. Will update.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say the Rollup version is always relevant. The second line could be:

  • Operating System + Node Version (or other Runtime Environment/Browser):

And I agree, the NPM version will probably not be relevant as all NPM specific functionality is usually handled by rollup-plugin-node-resolve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm down with removing the NPM version as a requirement, especially given how Rollup is shipped.

<!--
Issues without minimal reproductions will be closed! Please provide a repro by:
1. Using the REPL at https://rollupjs.org/repl, or
2. Provide a repository link (Read https://git.io/fNzHA for instructions)
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate repository links, as it always seems like a great way to put a backdoor onto a maintainers machine to me (npm install is executing).

2. Work to isolate the issue and write out the exact steps in the issue here.
3. Include a minimal GitHub repo link. These may take more time for us to look into than the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with this change. For consensus @lukastaegert please weigh in on it (Lukas had previously asked for the REPL and repo link in order from the template on master).

Copy link
Member

Choose a reason for hiding this comment

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

I think I am ok with @guybedford's change as it would still encourage the creation of a repo albeit with lower priority. From the debugging perspective, they can be a huge shortcut but I see the security concerns.

Maybe in the future we could work to extend the REPL to be able to reproduce more situations by adding a way to designate multiple entry points (maybe a simple checkbox above each module?) and some way (cogwheel button?) to provide a select set of config options that would make sense here.

### Feature Proposal


### Feature Use Case
Copy link
Contributor

Choose a reason for hiding this comment

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

Use case before proposal?


* If your pull request implements a new feature, please raise an issue to discuss it before sending code. In many cases features are absent for a reason.
* This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
* Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to `npm run lint`!)
Copy link
Contributor

Choose a reason for hiding this comment

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

These guidelines seem important to me to keep. Or perhaps reword into the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first point (about an accompanying issue) I've found that with new features, there's often more work done on implementation before discussion starts, if the contribution is coming from outside contributors, which is then discussed directly on the PR. I've also found it can be a hinderance to contribution if there's too much process but I'm completely OK with adding that back in. If you feel there's more value to the process than risk of dissuading contributions, I'm good with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can get behind what you're saying certainly in not crowding the process here.

Perhaps a note saying they should research previous discussion on the feature by searching the issues first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Definitely a step that should always happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed proposed additions based on that feedback.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Very nice! Two comments in bug (The one from Guy and one from me) that would be nice to have addressed but in general I am cool with using them.

- Rollup Version (if applicable):
- Operating System (or Browser):
- Node Version:
- NPM Version:
Copy link
Member

Choose a reason for hiding this comment

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

I'd say the Rollup version is always relevant. The second line could be:

  • Operating System + Node Version (or other Runtime Environment/Browser):

And I agree, the NPM version will probably not be relevant as all NPM specific functionality is usually handled by rollup-plugin-node-resolve.

However, the situation may arise where some small code snippets also need to
be provided. In that situation, please add your code below using
Fenced Code Blocks (https://help.github.com/articles/creating-and-highlighting-code-blocks/)
-->
Copy link
Member

Choose a reason for hiding this comment

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

👍


Hey there! If you need help or tech support then this is not the place to
ask. Please head to [the Rollup Gitter](https://gitter.im/rollup/rollup)
instead or post a question to https://stackoverflow.com/questions/tagged/rollupjs.
Copy link
Member

Choose a reason for hiding this comment

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

👍

while making animal noises.

Pull Request Requirements:
* Please include tests to illustrate the problem this PR resolves.
Copy link
Member

Choose a reason for hiding this comment

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

Huge thumbs up from me for making this the top priority! Hope at some point to write up a short guide to writing tests that we could link to from here.

@shellscape
Copy link
Contributor Author

OK I've reverted the (if applicable) on the Rollup version to avoid confusion with the Please do not skip message, and to go with Lukas' opinion that the Rollup version is always relevant. My opinion is the same, but not strongly held and was willing to change it to see how it felt. For the few issues where it may not be relevant I'm sure a user will put something in there to that effect (n/a or a question mark or something).

Also added the extra verbiage to the reproduction section in the BUG template, which I think reads fairly good now.

@shellscape shellscape merged commit 9b1d666 into master Aug 6, 2018
@shellscape shellscape deleted the chore/github-templates branch August 6, 2018 13:35
@shellscape shellscape restored the chore/github-templates branch August 6, 2018 14:20
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.

3 participants