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

Update PR Template #1075

Closed
joshbruce opened this issue Feb 24, 2018 · 3 comments
Closed

Update PR Template #1075

joshbruce opened this issue Feb 24, 2018 · 3 comments

Comments

@joshbruce
Copy link
Member

Bringing this over from the #1065

Sorry I've been a little absent and did a sloppy review, but I feel I have a couple of problems with this proposal.

The pull request template is a little confusing. First of all, all the checklist items regarding the release should not be there, but possibly in the RELEASE.md. They're confusing since they really have nothing to do with a PR that aims to solve some issue.
Plus I've already voiced my opinion that releases don't really need a PR, but I see that having the need for a review can help in making sure everything is okay before releasing. So it is important to submit the release PR, wait for a review, merge the PR, and only then publish on npm and create the release on github. (maybe make this clear in RELEASE.md?)

About the release PR, the things that needs to be checked are:

tests (covered by CI in the PR status)
linting (covered by CI in the PR status)
new version in package.json
marked.min.js is updated. I don't see the point of releasing without a code change unless it is the case only the package.json file need an update (like in #1054 but that is rare).
So all in all, three things need to be checked regarding the release PR by the reviewer:

CI status is green
package.json has the new version in it
marked.min.js is updated.
Whereas, in case of a contribution PR to solve some issue, I would treat it as if it were an "issue + patch" combination. So it either references some already submitted issue, or it presents the issue itself in the correct manner (expected/actual input/output samples):

specify which version of marked is affected (either by npm version or commit hash you're using)

if you're behind the current master, check that out and see if the issue has already been fixed (if you can)

if relevant, post clear input, expected output, and actual output:

input

if possible, type the input in babelmark and inspect the output by the different converters (notice that they're using an outdated marked version so don't take its output into consideration)

if an error is thrown, post the complete call stack and the markdown input

Then, for the code change, I would prepare the following checklist:

new tests added (if not necessary, list which tests cover this functionality)
old tests pass (run npm test)
code style is good (run npm run lint and correct any error messages before committing)
if feature proposal, document it clearly in the README
I would leave listing the other fixed issues to us collaborators since that is quite tricky.

So I guess these are my draft proposals for the issue and pull request templates, let me know!

@joshbruce
Copy link
Member Author

I like a lot of this and am on it!

Let me know some other thoughts, as they come.

@Feder1co5oave
Copy link
Contributor

There's no need for this separate issue, we can discuss things in the PR.

@joshbruce
Copy link
Member Author

@Feder1co5oave: I set the PR up to automatically close this Issue. Created the issue before the PR to get your feedback out of the previously merged PR. Suppose we could have referenced the merged PR, this way just made sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants