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

[Feature-241] Add PULL_REQUEST_TEMPLATE #242

Merged
merged 8 commits into from
May 24, 2023

Conversation

Radeity
Copy link
Member

@Radeity Radeity commented May 19, 2023

Purpose of the PR

Main Changes

  • Ditto.

Verifying these changes

  • This change is a doc work without any test coverage.

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - NO Need

@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #242 (14897e5) into master (cc5a7e7) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #242      +/-   ##
============================================
- Coverage     85.83%   85.80%   -0.04%     
+ Complexity     3232     3231       -1     
============================================
  Files           344      344              
  Lines         12072    12072              
  Branches       1087     1087              
============================================
- Hits          10362    10358       -4     
- Misses         1185     1187       +2     
- Partials        525      527       +2     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@imbajin
Copy link
Member

imbajin commented May 20, 2023

simplify it first, gradually adjust it according to the usage/feedback

image

Also the PR title use the basic rule first (consider expand it in future)

@liuxiaocs7 could take a look

@Radeity
Copy link
Member Author

Radeity commented May 20, 2023

Hi, @imbajin , IMHO, I don't recommend removing section Verifying these changes, how to test this pr is important, sometimes, it can not be tested by regular UT, also, it helps reviewer to know more about its degree of completion. Maybe we can simplify this section first.

@imbajin
Copy link
Member

imbajin commented May 20, 2023

Hi, @imbajin , I don't recommend removing section Verifying these changes, how to test this pr is important, sometimes, it can not be tested by regular UT, also, it helps reviewer to know more about its degree of completion. Maybe we can simplify this section first, WDYT?

Fine, use a short 🩳 description for it first, thanks for the feedback

BTW, the reason why I simplify the template is because in the “markdown source code” mode, most new users will directly “select all + delete” the content😿 (same in title), we had a similar problem with our previous issue template, until I used a new UI refactoring and did not allow it to be deleted (but the New Table UI can't use in PR template)

@Radeity
Copy link
Member Author

Radeity commented May 20, 2023

Fine, use a short 🩳 description for it first, thanks for the feedback

BTW, the reason why I simplify the template is because in the “markdown source code” mode, most new users will directly “select all + delete” the content😿 (same in title), we had a similar problem with our previous issue template, until I used a new UI refactoring and did not allow it to be deleted (but the New Table UI can't use in PR template)

Get your point :D, and i've modified some description, maybe this part can be improved, PTAL: https://github.com/Radeity/incubator-hugegraph-computer/blob/31ea3e02b731255f42e877dd82ee45d0a1f5182d/.github/PULL_REQUEST_TEMPLATE.md?plain=1#L51-L54

coderzc
coderzc previously approved these changes May 22, 2023
liuxiaocs7
liuxiaocs7 previously approved these changes May 22, 2023
@Radeity
Copy link
Member Author

Radeity commented May 22, 2023

nit: should we use close: #issue_number here to be consistent with L7 and L26

Hi, @liuxiaocs7 , thanks for reminding, I think use link here is better, so i've modified comment in L7 and L26 :D

@coderzc coderzc requested a review from liuxiaocs7 May 23, 2023 13:21
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

LGTM, unify it in other repo later

@liuxiaocs7
Copy link
Member

liuxiaocs7 commented May 23, 2023

LGTM, unify it in other repo later

Added in HG Tasks, good start for beginner by refering this pr!

@coderzc coderzc merged commit c806528 into apache:master May 24, 2023
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.

[Feature] Add PULL_REQUEST_TEMPLATE
5 participants