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

Add GitHub issue form templates #4

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Add GitHub issue form templates #4

merged 1 commit into from
Aug 30, 2022

Conversation

leandro-lucarella-frequenz

Add a form for reporting bugs and requesting new features. Also configures the new issue screen to show both templates plus a quick access to ask questions in the Discussions forum.

I'm not so sure about the version:xxx labels (and the form entry). I think for this repo it might be overkill but eventually for other repos it might be useful, specially when bug reports come from external users, is a good way to make users think about which version are they using (and thus check if it is the latest one).

If this becomes too annoying (as with all the latest PRs), we can change it.

For more info on GitHub issue forms: https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms

@leandro-lucarella-frequenz leandro-lucarella-frequenz added type:tech-debt Improves the project without visible changes for users version:0.10.x labels Aug 26, 2022
@leandro-lucarella-frequenz leandro-lucarella-frequenz added this to the v0.11.0 milestone Aug 26, 2022
@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Aug 26, 2022
Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz left a comment

Choose a reason for hiding this comment

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

A few grammatical fixes to address here and there, but looks good overall.

.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
@leandro-lucarella-frequenz
Copy link
Author

Thanks for all the corrections! It would have been awesome if you provided them in form of change suggestions! 😄 (me lazy?)

image

All done! I'm also trying out the approach of adding new commits after review comments instead of amending the existing commits. This (ideally) should make it easier to review a PR after it was modified based on comments in a review ♾️. If you hate it I can amend the existing commit too.

Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz left a comment

Choose a reason for hiding this comment

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

Sorry, one last nitpick :)

I like the idea of using suggestions!

.github/ISSUE_TEMPLATE/bug.yml Outdated Show resolved Hide resolved
@tiyash-basu-frequenz
Copy link
Contributor

I'm also trying out the approach of adding new commits after review comments instead of amending the existing commits. This (ideally) should make it easier to review a PR after it was modified based on comments in a review

I you overwrite the commits in a PR, then github shows you the diff from the force-push. It may not be as granular as individual commits, but it gets the job done for me. So, for correcting small mistakes like typos and such, I'd personally prefer to overwrite existing commits, in order to keep the commit history clean. :)

@sahas-subramanian-frequenz

I'm also trying out the approach of adding new commits after review comments instead of amending the existing commits. This (ideally) should make it easier to review a PR after it was modified based on comments in a review infinity. If you hate it I can amend the existing commit too.

this is what fixup commits are for. They make it easy to review, and can be squashed before merging.

@leandro-lucarella-frequenz
Copy link
Author

I you overwrite the commits in a PR, then github shows you the diff from the force-push. It may not be as granular as individual commits, but it gets the job done for me. So, for correcting small mistakes like typos and such, I'd personally prefer to overwrite existing commits, in order to keep the commit history clean. :)

I think is a shift of workflow, you know pretty well I used to work like that and I really liked, now I tried this way and I think it has a few advantages. Besides GitHub being better at it (for example links to old reviews break if you amend and force push, and getting better tracking of what you already reviewed and what you didn't), I'm not sure what you want a clean history for. You are even hiding the history that way (for example, if you want to see in the git history how much effort a change took, it is very deceiving if you see only one perfect commit rather 3 or 4 with some subtle fixes that weren't obvious at first). The history is I wrote something, you saw some problems and suggested some changes, and I did those changes. If you want a summarized view of the history, you can use git log --merges and you only get the merges which tells the summary.

this is what fixup commits are for. They make it easy to review, and can be squashed before merging.

I would argue that that's one way fixup commits can be used. It would be much better if GitHub had an option to enable --autosquash when merging, otherwise I have to do it manually (same with applying review suggestions). But again, that doesn't solve the problem of losing the history of how a change evolved thanks to the feedback from peers.

But is OK, I was also used to do rebases and was quite convinced it was the best (remember I even wrote a CLI tool to do "rebase merges" before GitHub supported it 😆), and I know it takes time to do a switch, so I'm fine with continuing with it too :)

New suggestion applied. I will squash now.

Add a form for reporting bugs and requesting new features. Also
configures the new issue screen to show both templates plus a quick
access to ask questions in the Discussions forum.

Signed-off-by: Leandro Lucarella <leandro.lucarella@frequenz.com>
@sahas-subramanian-frequenz

But again, that doesn't solve the problem of losing the history of how a change evolved thanks to the feedback from peers.

I agree, and if we have external contributors, it will be hard to convince them to keep commit history beautiful. I'm glad we have merge commits now so what people do in their branches becomes less important.

@tiyash-basu-frequenz
Copy link
Contributor

I think it really depends on the preferences of the contributors, whether they want to squash commits or not. The reason is that we can't control what rules contributors have set for their branches, and everyone will have a different definition of a clean or accurate history.

So, I am happy with either approach.

Copy link
Contributor

@tiyash-basu-frequenz tiyash-basu-frequenz left a comment

Choose a reason for hiding this comment

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

LGTM!

@leandro-lucarella-frequenz leandro-lucarella-frequenz merged commit 1ae02cb into frequenz-floss:v0.x.x Aug 30, 2022
@leandro-lucarella-frequenz leandro-lucarella-frequenz deleted the issue-templates branch August 30, 2022 10:13
Copy link

@andrew-stevenson-frequenz andrew-stevenson-frequenz left a comment

Choose a reason for hiding this comment

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

Just pednaticness

attributes:
label: What's needed?
description:
Please tell us what's missing or what could be done better or easier.

Choose a reason for hiding this comment

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

Suggested change
Please tell us what's missing or what could be done better or easier.
Please tell us what's missing or what could be done better or more easily.

label: What's needed?
description:
Please tell us what's missing or what could be done better or easier.
placeholder: What's missing or what could be done better or easier.

Choose a reason for hiding this comment

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

Suggested change
placeholder: What's missing or what could be done better or easier.
placeholder: What's missing or what could be done better or more easily.

attributes:
label: Proposed solution
description:
Please tell us how do you think the needs above can be fulfilled. Only

Choose a reason for hiding this comment

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

Suggested change
Please tell us how do you think the needs above can be fulfilled. Only
Please tell us how you think the needs above can be fulfilled. Only

attributes:
label: Alternatives and workarounds
description:
Please tell us if you tried any alternatives or workarounds for those

Choose a reason for hiding this comment

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

Suggested change
Please tell us if you tried any alternatives or workarounds for those
Please tell us if you tried any alternatives or workarounds for these

label: Alternatives and workarounds
description:
Please tell us if you tried any alternatives or workarounds for those
use cases and how (un)useful they are.

Choose a reason for hiding this comment

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

Suggested change
use cases and how (un)useful they are.
use cases and how (un)useful they were.

Please tell us if you tried any alternatives or workarounds for those
use cases and how (un)useful they are.
placeholder:
Any alternatives or workarounds for those use cases and how (un)useful

Choose a reason for hiding this comment

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

Suggested change
Any alternatives or workarounds for those use cases and how (un)useful
Any alternatives or workarounds for these use cases and how (un)useful

use cases and how (un)useful they are.
placeholder:
Any alternatives or workarounds for those use cases and how (un)useful
they are.

Choose a reason for hiding this comment

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

Suggested change
they are.
they were.

attributes:
label: Additional context
description:
Please add any addition information here, screenshots, diagrams, etc.

Choose a reason for hiding this comment

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

Suggested change
Please add any addition information here, screenshots, diagrams, etc.
Please add any additional information here - screenshots, diagrams, etc.

label: Additional context
description:
Please add any addition information here, screenshots, diagrams, etc.
placeholder: Any addition information here, screenshots, diagrams, etc.

Choose a reason for hiding this comment

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

Suggested change
placeholder: Any addition information here, screenshots, diagrams, etc.
placeholder: Any additional information here - screenshots, diagrams, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) type:tech-debt Improves the project without visible changes for users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants