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

put the actual description first in issue templates #35295

Merged
merged 5 commits into from
Jul 9, 2023

Conversation

mezzarobba
Copy link
Member

...so that relevant information appears when hovering over items of the
issue list, and so that one can read the actual issue on the issue page
without scrolling over a full page of boilerplate.

Also remove the title from the Description section of the PR template
(same rationale).

Any opinions on that?

📚 Description

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

...so that relevant information appears when hovering over items of the
issue list, and so that one can read the actual issue on the issue page
without scrolling over a full page of boilerplate.

Also remove the title from the Description section of the PR template
(same rationale).
...that don't make sense with their particular PR
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

I agree that the checkboxes should not come first, so it looks better now.

@mezzarobba
Copy link
Member Author

mezzarobba commented Mar 16, 2023

Thanks for your comment!

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

From a dev point of view that mainly reads issues, this seems like a good and helpful change. The rationale for the checklist-first approach is that it serves as a guideline for people creating issues. If the checklist is at the end, there is a high chance that it is simply ignored (are you going to really search for similar issues if you already spent 5 mins writing the main issue body?)

Not sure what is more important...

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
@mezzarobba
Copy link
Member Author

Thanks for your comments.

If the checklist is at the end, there is a high chance that it is simply ignored

I agree that this is a risk, but we can always go back to the old version if this proves an issue in practice.

As a middle ground, we could also leave the instructions (“Please search to see if an issue already exists for the bug you encountered.”, etc.) at the beginning while moving the checklist itself to the end.

As an aside, I don't really like the fact that many fields, including the checklist items, are mandatory. My experience with forms is that there are always cases where you really have a good reason not to fill a given field but the computer doesn't let you...

@tobiasdiez
Copy link
Contributor

As a middle ground, we could also leave the instructions (“Please search to see if an issue already exists for the bug you encountered.”, etc.) at the beginning while moving the checklist itself to the end.

That sounds like a really nice solution, indeed!

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (f449b14) 88.62% compared to head (d6d7630) 88.61%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35295      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398653   398653              
===========================================
- Hits        353308   353264      -44     
- Misses       45345    45389      +44     

see 23 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mezzarobba mezzarobba marked this pull request as ready for review March 17, 2023 09:11
...while keeping the checklist at the end.

With this version, I thought it made sense to apply the change to
ftbfs issues too. I left the "Environment" section near the beginning in
that case, though it makes for a small inconsistency wrt the bug report
template.
@mezzarobba
Copy link
Member Author

I have implemented the solution discussed with Tobias and removed the draft flag. Formal review welcome!

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@mezzarobba
Copy link
Member Author

Thank you!

@vbraun
Copy link
Member

vbraun commented Mar 26, 2023

Merge conflict

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Documentation preview for this PR (built with commit f2a3896; changes) is ready! 🎉

@mezzarobba
Copy link
Member Author

@mkoeppe, I'm not sure why you added the needs_work label after Dima fixed the merge conflict. I reverted the ticket to positive review, but feel free to change it again if there was indeed something to fix.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 3, 2023

I don't remember; probably I just didn't notice that Dima had already fixed the conflict, or it happened simultaneously.

@vbraun vbraun merged commit f6e3c53 into sagemath:develop Jul 9, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 9, 2023
@mezzarobba mezzarobba deleted the issue_template branch December 6, 2023 11:01
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.

6 participants