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

Contributing.md link & PR template simplifications & Added ISSUE_TEMPLATE.md #1399

Merged
merged 16 commits into from
May 9, 2022

Conversation

xBalbinus
Copy link
Contributor

Closes: #1263 (Not merging until this also gets closed on docs side)

What is the purpose of the change

  • There's a bunch of stuff that if in the current PR template that we can simplify and make smoother.

Brief change log

  • Added a link to the upcoming "contributing.md" located soon on both the docs and this current repo
  • Changed current PR template to be smoother (less scrolling for everyone!)

Tests / Documentation

  • No tests altered, documentation soon to be PR'd onto docs.

@xBalbinus xBalbinus requested review from daniel-farina, czarcas7ic and a team May 3, 2022 18:10
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2022

Codecov Report

Merging #1399 (0ff98ce) into main (f56fbe5) will decrease coverage by 0.29%.
The diff coverage is 17.15%.

@@            Coverage Diff             @@
##             main    #1399      +/-   ##
==========================================
- Coverage   19.82%   19.53%   -0.30%     
==========================================
  Files         202      226      +24     
  Lines       27685    30950    +3265     
==========================================
+ Hits         5489     6045     +556     
- Misses      21175    23800    +2625     
- Partials     1021     1105      +84     
Impacted Files Coverage Δ
x/epochs/client/cli/query.go 0.00% <ø> (ø)
x/gamm/client/cli/query.go 36.74% <0.00%> (-0.36%) ⬇️
x/gamm/pool-models/stableswap/amm.go 40.27% <0.00%> (-15.50%) ⬇️
x/gamm/pool-models/stableswap/msgs.go 0.00% <0.00%> (ø)
x/gamm/pool-models/stableswap/pool.go 0.00% <0.00%> (ø)
x/gamm/pool-models/stableswap/stableswap_pool.go 0.00% <0.00%> (ø)
.../gamm/pool-models/stableswap/stableswap_pool.pb.go 0.58% <0.00%> (-0.09%) ⬇️
x/gamm/types/pool.go 0.00% <ø> (ø)
x/incentives/client/cli/query.go 0.00% <ø> (ø)
x/incentives/keeper/distribute.go 61.00% <0.00%> (ø)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d812215...0ff98ce. Read the comment docs.

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Nice work. I like the issue template. Left a couple of comments too, let me know what you think

.github/issue_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
Comment on lines 46 to 51
## Documentation and Release Note

- Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
- Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? (yes / no)
- How is the feature or change documented? (not applicable / specification (`x/<module>/spec/`) / [Osmosis docs repo](https://github.com/osmosis-labs/docs) / not documented)

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this one was better, it forced acknowledgement of the options for docs updates

Comment on lines 27 to 40

*(Please pick one of the following options)*

This change is a trivial rework / code cleanup without any test coverage.

*(or)*

This change is already covered by existing tests, such as *(please describe tests)*.

*(or)*

This change added tests and can be verified as follows:

*(example:)*
Copy link
Member

Choose a reason for hiding this comment

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

Do others dislike this? I thought this was great, I just choose the one I want and delete everything else


## Brief change log

## Brief change log (include any info relevant to tests))
Copy link
Member

Choose a reason for hiding this comment

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

what is (include any info relevant to tests) for?

@xBalbinus
Copy link
Contributor Author

xBalbinus commented May 3, 2022 via email

README.md Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
xBalbinus and others added 2 commits May 3, 2022 17:07
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@xBalbinus
Copy link
Contributor Author

Alright folks! Reverted changes to pull_request_template.md but issue_template.md mostly remains the same. Also fixed @alexanderbez 's comment on README.md. Let me know what you all think :)

.github/pull_request_template.md Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this exist under .github/ISSUE_TEMPLATE? Also, what kind of template is this? For bugs? I propose something like the following: https://github.com/umee-network/umee/blob/main/.github/ISSUE_TEMPLATE/bug-report.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't happen to know we had a .github/ISSUE_TEMPLATE already if we did (I don't happen to see it), I can definite create a /ISSUE_TEMPLATE folder though. And this was originally supposed to be for generic issues, not specifically bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should have templates per type IMO (i.e. bugs, general proposal, features, etc...). Otherwise, having a template isn't all that useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll make that then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think general proposals / features are done in a separate repo, and questions are done in our discord server, so I currently only implemented the bug reporter template, the generic issue template along with the option to still open blank issues. Let me know your thoughts :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think bug report and general template is fine. I don't really understand why features are done in a separate repo, but I don't wanna bike shed here.

.github/ISSUE_TEMPLATE/issue.md Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
blank_issues_enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing new line at the end

@xBalbinus
Copy link
Contributor Author

Just wondering, has this been merged in yet?

xBalbinus and others added 3 commits May 9, 2022 12:00
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@xBalbinus xBalbinus merged commit 524a2db into main May 9, 2022
@xBalbinus xBalbinus deleted the xiangan/contributing branch May 9, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

add CONTRIBUTING.md rather than using gaia's
5 participants