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

docs: add a PR template #391

Merged
merged 4 commits into from
Apr 19, 2024
Merged

docs: add a PR template #391

merged 4 commits into from
Apr 19, 2024

Conversation

joanise
Copy link
Member

@joanise joanise commented Apr 17, 2024

Q A
Goal? Add a PR template so PR submitters provide more information
Fixes? N/A
Feedback sought? Is this format OK?, are the additional details in comments annoying or useful?
Priority? normal
Tests added? N/A
How to test? Won't work until merged, but can be tested on my just-testing repo
Confidence? medium - format is not perfect, and I wish it aligned better where the user is filling out this form.

Do you think this PR should trigger a Major (Breaking Change)/Minor (New Feature)/patch (refactor/bug fix) version change? No

Copy link
Contributor

github-actions bot commented Apr 17, 2024

CLI load time: 0:00.27
Pull Request HEAD: 48def6c541157b26c9ecab5dc7cbedb6dfc82cff
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

@joanise
Copy link
Member Author

joanise commented Apr 17, 2024

Note: the table is now weirdly mis-aligned when you look at it in fixed font, but that makes it roughly align in the default view you get when you use it to submit a PR, since GH uses a variable-font editor.

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.81%. Comparing base (7d3d8de) to head (48def6c).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #391      +/-   ##
==========================================
- Coverage   73.15%   71.81%   -1.34%     
==========================================
  Files          43       43              
  Lines        2786     2973     +187     
  Branches      459      520      +61     
==========================================
+ Hits         2038     2135      +97     
- Misses        664      749      +85     
- Partials       84       89       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@roedoejet roedoejet left a comment

Choose a reason for hiding this comment

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

just one comment, thanks @joanise !

| Q | A
| ----------------- | ---
| Goal? | <!-- Explain the main objective of this PR. -->
| Fixes? | `Fixes #1, Fixes #2` <!-- Remove the (`) quotes and write "Fixes" before the number to link the issues. -->
Copy link
Member

Choose a reason for hiding this comment

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

the pipes are not aligned here - can we format it so that even in the raw text format they appear aligned? Also I think we can expand Q and A to "Question" and "Answer since we have space.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we align them in fixed width, they look like this in the PR comment editor:
image
What I did here was align them as best as I could in the PR comment editor, so now they look ugly in the fixed width view. I don't know how to make it align OK in both views.

Agreed for spelling Question and Answer in full.

Copy link
Member Author

Choose a reason for hiding this comment

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

By contrast, this is what it looks like with the current version:
image

I could move it to this:
image

but I don't think we'll like it if I make it align perfectly in fixed width.

Copy link
Member

Choose a reason for hiding this comment

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

ok it doesn't look like that for me when I click on your link to the PR:
image

@joanise
Copy link
Member Author

joanise commented Apr 19, 2024

Rewritten with visible headings instead of a table that I can't get to align nicely.
Test link for this variant: https://github.com/joanise/just-testing/compare/main...dev.gzip-on-ci-all-OSs?expand=1&template=visible_headings.md

Copy link
Member

@roedoejet roedoejet left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for continuing to wrestle with the formatting @joanise !

@joanise joanise merged commit 51d0e06 into main Apr 19, 2024
3 of 4 checks passed
@joanise joanise deleted the dev.ej/pr-template branch April 19, 2024 21:26
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.

None yet

2 participants