Skip to content

Add a bug report template. (DO NOT MERGE) #1244

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

Merged
merged 43 commits into from
Apr 15, 2019
Merged

Add a bug report template. (DO NOT MERGE) #1244

merged 43 commits into from
Apr 15, 2019

Conversation

HonkingGoose
Copy link
Contributor

@HonkingGoose HonkingGoose commented Apr 4, 2019

🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧

Hi @ben, @schacon, @jnavila,

I think that there is a lot to be gained by adopting templates for bugs/features/pull-requests.
I'll explain why I think this is a good idea below.

I think helping contribute to a project should be easy and clear. Templates are a good way to achieve this. A template can have helpful guidance and a checklist of common pitfalls/errors. If contributors follow the checklist, and follow the template, they should end up with a good bug report/feature request/pull request. This takes the guesswork out of contributing.

During my efforts to clean up the old issues on the issue tracker, I've noticed that sometimes people file multiple bugs in one bug report. This makes it hard to diagnose/track the issue(s). A bug report should have a single actionable bug. That way it's easy to track, fix and close.

We should also remind people not to file a duplicate bug, or file the bug in the wrong place, I've added a checklist that people can mark with x to show they checked it. This will also reduce the frustration of making a good effort attempt, only to find out later that you've filed the bug in the wrong place, or made a duplicate and it gets closed.


Below is a starting point for a bug report template. This is work in progress, and is open to feedback.

I hope you all find this helpful. If not, feel free to say so, and I'll leave it be. 👍

🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧

@jnavila
Copy link
Member

jnavila commented Apr 9, 2019

How do you feel about this PR?

@HonkingGoose
Copy link
Contributor Author

HonkingGoose commented Apr 10, 2019

Hi @jnavila are you asking me how I feel about this PR? Or are you asking @ben and @schacon how they feel about my PR?


As for my feelings on my PR:
This is still somewhat work in progress. But I'm not sure how to improve on the PR further. Some points I'm not sure about:

  • Merge the Desktop and Smartphone section into one. That would save some space on the template. Perhaps something like this would be better?
**Device used**
 - Smartphone: <!-- [e.g. iPhone X, Samsung Galaxy S10] Write N/A if using desktop/laptop.-->
 - Desktop/laptop: <!-- Write "yes" if using a Desktop/laptop. Write N/A if using a smartphone. -->
 - Operating System: <!-- [e.g. iOS12.2, Ubuntu 19.04, Windows 10, Mojave 10.14] -->
 - Browser: <!-- [e.g. Firefox 66.0.2, Safari 12.1, Chromium 73.0.3683.103] -->
  • Update the references to software/hardware to more recent versions.
  • I would also like some general style/tone feedback. 😄

Copy link
Member

@jnavila jnavila left a comment

Choose a reason for hiding this comment

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

We don't really care about OS, but the applications and the book formats are very precious.

@HonkingGoose
Copy link
Contributor Author

HonkingGoose commented Apr 13, 2019

@jnavila This pull request already contains 41 commits, and is likely to have more commits before we finalize it.

  • Do you want a clean pull-request with just 1 initial commit for the finalized version?
  • Or do you intend to squash or rebase this PR when you merge it into progit2/master?

@jnavila
Copy link
Member

jnavila commented Apr 13, 2019

Please create another PR with one squashed commit and we'll merge it.

@HonkingGoose HonkingGoose changed the title Add a bug report template. Add a bug report template. (DO NOT MERGE) Apr 13, 2019
@HonkingGoose
Copy link
Contributor Author

@jnavila I've pushed two more commits (e5089e1 and c8da430), I this the final result you wanted to have, or did I misunderstand something?

@aollier
Copy link
Contributor

aollier commented Apr 15, 2019

@HonkingGoose Currently you have pushed 43 commits, this is too much. What @jnavila told you to do is to merge your changes in a single commit.
You can do that by rebasing from the first commit and squash all your commits into a single one. Then you'll have to find a good message for the commit.

@aollier
Copy link
Contributor

aollier commented Apr 15, 2019

Then you'll have to push --force.

@HonkingGoose
Copy link
Contributor Author

Hi @aollier,

I know merging 43+ commits messes up the git log history, and is of no use to anybody later. I agree with you that it's better to have one clean commit. 👍

The reason that there are still 43 commits on this PR is because I'm awaiting @jnavila's feedback regarding my last 2 commits (e5089e1 and c8da430). I'm awaiting his "blessing" so to speak, so that I know the template is done. After I have the "blessing", I'll clean up this mess of commits. 😄

Now assuming I get the "blessing", which way is the best way to proceed?

  1. Open new PR, with only one final commit. (@jnavila's advice)
  2. Rebase and squash, then force push on top of current PR. (@aollier's advice)

Thanks for the advice on how to rebase/squash and then force push. That's always good to know. And thank you for giving me feedback and getting things moving. 😄

Greetings,

HonkingGoose

@ben
Copy link
Member

ben commented Apr 15, 2019

I personally don't mind having lots of commits, but if this is a point of sensitivity we can always just do this.

image

@jnavila
Copy link
Member

jnavila commented Apr 15, 2019

I'm blessing this PR 🤡 🤴 . I've never tried the "squash and merge" on GitHub, but I guess this will loose the attribution of commit, which is not fair for @HonkingGoose , plus I usually merge outside of the web interface (old school style...). So, if @HonkingGoose can squash and force-push, I'll be glad to merge ASAP.

@HonkingGoose
Copy link
Contributor Author

@jnavila Thanks for blessing the PR 🤡.


I'm getting very very stuck on the git rebase thing. 😟 So if somebody can get me unstuck, I would be very grateful.


I've read the chapter https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History and https://git-scm.com/book/en/v2/Git-Branching-Rebasing but neither of them seems directly applicable to my situation.

It does say:

You can run rebase interactively by adding the -i option to git rebase. You must indicate how far back you want to rewrite commits by telling the command which commit to rebase onto.

But that is still a bit too opaque for me to understand unfortunately. It doesn't make it clear for me how to select the correct SHA1 hash to give to the git rebase -i command.


git rebase -i [sha1] needs a SHA1 hash to start from, but which SHA1 hash should that be?
b289251 (last "updated" merge from master) or a5d0804 (start of work on bug template). The git rebase -i [sha1] command is not explained in the Pro Git book, only git rebase -i HEAD~3 is explained.

Should I pull the changes made in master into my branch before rebasing my branch? My branch is not up to date with master.

I'm probably massively overcomplicating things here... 😟


Below is the current output of git log, notice that my branch is not current with progit2/master yet!

git log --oneline --graph --decorate --pretty=oneline
* c8da430 (HEAD -> HonkingGoose-bug-report-template, origin/HonkingGoose-bug-report-template) Squash comments into one comment above each list.
* e5089e1 Remove e.g.
* 6a71ead Remove horizontal ruler.
* e1eb5a0 Add colon to headers.
* fc54e47 Comment out checklist. Remove ruler.
* 0c0ccca Add horizontal rulers. Move context section up.
* c6b8454 Add comment for device detail forms.
* 7741ce0 Clarify why we need the source/file comparison.
* 97be0ac Expand list to include epub/mobi files.
* 2eec9e1 Remove line about not accepting .epub/.mobi bugs.
* fc97286 Fix grammar.
* d7e4d08 Rewrite pdf/website question.
* eb081ac Rewrite text to better fit list format.
* cb9de83 Convert checkbox pdf/website to a question.
* e191fe6 Convert checkboxes to list items.
* 6356959 Change wording for pdf/website cross-check.
* a424fdc Change wording for checkbox.
* 45d9adb Fix indention for all list items.
* 1f4e330 Fix indentation.
* f35e38a Remove text.
* ff3de15 Add checkbox/selector for pdf/website.
* 8a70a99 Fix comment.
* 2750a22 Add application.
* d118c7a Add application.
* f602982 Fixes, see description below
* a06effa Add section on e-book readers.
* a588788 Create inline links.
* bdff49c Add checkbox, fix text.
* 1bf0d1d Remove backticks.
* ad43927 Add Javascript comment.
* a222057 Add colon.
* 5d62032 Fix Javascript comments for list.
* 3a242ef Change text to be more descriptive.
* a94f3e1 Add how-to mark checkbox with Github Markdown.
* 802d034 Add note to refence other bugs.
* 27ad71c Add guidance on steps to reproduce.
* 2b93405 Remove unnecessary capitalization.
* 81843b5 Remove unnecessary capitalization.
* ae87eaf Add Javascript comments, add punctuation.
* 18d4d2a Replace list with table.
* e727a5b Add link to translation projects.
* 2c18789 Add checklist.
* a5d0804 Update issue templates
*   b289251 Merge pull request #1240 from HonkingGoose/arabic_translation
|\  
| * bc23c02 Add Arabic translation.
|/  
*   b9b0cdf Merge pull request #1239 from HonkingGoose/persian_translation
[snip]

@ben
Copy link
Member

ben commented Apr 15, 2019

Tell you what, let's give this merge-as-squash thing a try. If we don't like it we can always revert and run these commits again. You should still be shown as the author for that commit, so your name will show up in the blame view.

@ben ben merged commit cb5a2d6 into progit:master Apr 15, 2019
@ben
Copy link
Member

ben commented Apr 15, 2019

💥 cb5a2d6. I hope that satisfies.

The other way you'd do a rebase looks like this, which sounds like a lot but isn't that bad once you've done it 3 or so times:

  1. Fetch from the remote that represents this repo (git fetch upstream)
  2. Start an interactive rebase on the newly-fetched master (git rebase -i upstream/master)
  3. In the resulting editor, change every occurrence of pick except the first to fixup (or f)
  4. Change the first pick to reword (or r)
  5. :wq
  6. The next editor is for the final commit message. Change the contents to something reasonable and :wq

Again, that sounds hard, but if you've done it before it's not so bad, and the results are rather nice.

@HonkingGoose
Copy link
Contributor Author

Hi @ben, thanks for trying the merge-as-squash. I noticed it says:

HonkingGoose authored and ben committed

That's perfectly fine by me for my credit. 🎉


However the git log now says: Add a bug report template. (DO NOT MERGE) (#1244) I'm not sure that's the cleanest log entry now. It looks like we merged something that shouldn't be merged. 😄

@HonkingGoose HonkingGoose deleted the HonkingGoose-bug-report-template branch April 20, 2019 22:12
@minchen085
Copy link

求救

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.

5 participants