Try to simplify the PR template a bit - Fix #449

[Malabarba]

This shortens the template just a bit.

• It removes the "make test" line. That's why we have CI, and running tests locally can indeed be quite cumbersome.
• It removes the unexplained bytecode thingy (since that's included in make tests).
• It says that writing tests is optional. We can always ask the person to write a test after they open the PR (and they're much more likely to write it at that point).

---

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s). Indentation & font-lock tests are extremely important!
• All tests are passing (make test)
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)

Thanks!

[bbatsov]

I don't see how this reworded version is better than the original.

[Malabarba]

It was just to add the word "optionally".

[bbatsov]

Same here.

[Malabarba]

True, I can change it back.

[bbatsov]

I agree with the first 2 points, I'm not sure about the third. Probably we can also automate checkdoc. I actually thought we did it at some point, but it might have been for CIDER.

[Malabarba]

Ok, I can revert the two comments, and leave only the first two points.

[bbatsov]

Fine by me.

[Malabarba]

Actually, I looked into it now and we have indeed automated checkdoc. :-)
Though it looks like everything is broken on travis for some issue with the setup.

[Bost]

It removes the "make test" line. That's why we have CI, and running tests locally can indeed be quite cumbersome.

I think we should put more emphasis on having test results. So to say No PR w/o test logs. If travis is broken then the ERT tests **must be** done by hand. (It looks like the PR I made some days ago is buggy and I didn't attach the ERT unit tests so now I'm poking around it the fog.)

[bbatsov]

You didn't change the punctuation here as you did with the previous items. The punctuation should be consistent.

[bbatsov]

Seems you deleted this blank line by accident.

[timvisher]

@Malabarba I don't have much (any?) involvement in this project so disregard my comments if they're not welcome. :)

This removes make test because "Running tests locally can [be cumbersome]". Can you elaborate on that? Either here or in an issue? I did check the project out and run make test and it immediately failed because I don't have cask. I don't even know what cask is. :) However I do have a significant amount of experience making this sort of thing work much more easily across multiple devs/platforms. If I knew what the pain points were I might be able to contribute making it so that make test is expected to work before you submit your PR. That's more along the lines of a normal development workflow IMO.

Or disregard this, like I said. I know you're busy. I appreciate all the work you all do anyway. :)

[Bost]

I might be able to contribute making it so that make test is expected to work before you submit your PR.

Be reminded please that cask has at the moment 66 opened issues (See e.g. one of the bigger ones - the #373.) I'd say it would be more viable to do the tests with something else that the cask.

[Malabarba]

@timvisher If you'd like to help WRT to automation, would you be willing to investigate why our travis builds are broken? I think that's something that would provide a lot of value right now.

The reason I'd like to remove the make test item is primarily because it is (or should be) unnecessary. It doesn't waste our time if someone submits a PR with a broken test, as long as the automated builds catch that and notify automatically.

[Malabarba]

I've addressed the inconsistencies pointed out by Bozhidar (and made the phrasing more consistent), but I'll wait to see if we can fix the builds before I merge this.

[bbatsov]

👍

[timvisher]

@Malabarba I'll try to take a look at the travis builds if I can find some time. This might be a difference in philosophy but I believe that the CI build should never break. I don't like processes that emphasize letting another system tell me that the tests are broken mainly because it's too long of a feedback loop.

[Malabarba]

@timvisher maybe it's just a difference in contexts. I work at a place where tests will take 15 minutes to run, so I prefer to push my changes and start doing something instead of just watching the tests run for 15 minutes.

[timvisher]

@Malabarba Ah that makes sense. Generally I push long running tests into integration territory and yeah I just let CI run those for the most part. Hopefully the CIDER tests don't take 15 minutes to run. :)

I still haven't had a chance to look into this yet but it's still on my list. Hope I can help.