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

[Improvement][Style] add autolink to issues in PR title and commit msg #551

Closed
3 tasks done
advancedxy opened this issue Feb 6, 2023 · 18 comments · Fixed by #588
Closed
3 tasks done

[Improvement][Style] add autolink to issues in PR title and commit msg #551

advancedxy opened this issue Feb 6, 2023 · 18 comments · Fixed by #588

Comments

@advancedxy
Copy link
Contributor

advancedxy commented Feb 6, 2023

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

What would you like to be improved?

It's common and considered best practice to be able to click links to corresponding issues/PRs/tickets in the commit msg. To achieve this features, Github's autolinked references and links1 are used.

For example, Spark uses this feature to autolink Jiras.
image

Since Uniffle are using Github Issues to track issues/request/improvements, etc. The issue is effective the same as the Jira ticket.

I'd like to propose an suggestion to improve the code context experience: add autolink to issues in PR title and commit msg.

How should we improve?

  1. Let's decide which form to include autolinks:
    • [#xxx], the simple form, cf9afab
      image
    • [ISSUE #xxx] prefixed with the ISSUE
  2. Enforce the pr title check. Every PR should have an issue linked.
    There's an exception rule for this: PR title prefixed with [Minor]/[MINOR] can be excused for this enforcement check.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Footnotes

  1. https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls

@advancedxy
Copy link
Contributor Author

@kaijchen @zuston @jerqi @felixcheung WDYT?

@zuston
Copy link
Member

zuston commented Feb 6, 2023

Can we use the apache style like [UNIFFLE-120][Test] Fix msg ? And the uniffle-120 could be linked to github issue by https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/configuring-autolinks-to-reference-external-resources

@kaijchen
Copy link
Contributor

kaijchen commented Feb 6, 2023

UNIFFLE-120 may be confusing, because it's usually a Jira id.
Currently I would not recommend Jira because the self registration is limited. We wish to make new contributors easier to join the project.
For how to configure the autolink, see https://cwiki.apache.org/confluence/display/INFRA/Git+-+.asf.yaml+features

@advancedxy
Copy link
Contributor Author

Can we use the apache style like [UNIFFLE-120][Test] Fix msg ? And the uniffle-120 could be linked to github issue by https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/configuring-autolinks-to-reference-external-resources

No. The asf doesn't expose the configurations to external resource except JIRA which is already been closed for public registration.

I'd like to use [ISSUE-#xxx], but it seems Github doesn't recognize that. You have to use [ISSUE #XXX] or [Uniffle #xxx] or just [#xxx]

@kaijchen
Copy link
Contributor

kaijchen commented Feb 6, 2023

I would suggest omit issue id in the title, and just add it in the PR description.

### Why this change needed?

Fix #123

The PR title could be [module][type] description.

Modules:
Info of who (module owner) should review this pr.

  • coordinator
  • server
  • client
  • operator
  • *: all
  • etc

Types:
Implies the urgency, the review time required, and whether should be backported to release branches.

  • feature: (new feature for the user, not a new feature for build script)
  • fix: (bug fix for the user, not a fix to a build script)
  • docs: (changes to the documentation)
  • style: (formatting, missing semi colons, etc; no production code change)
  • refactor: (refactoring production code, eg. renaming a variable)
  • test: (adding missing tests, refactoring tests; no production code change)
  • chore: (updating grunt tasks etc; no production code change)

@kaijchen kaijchen changed the title [Improvement][CodingStyle] add autolink to issues in PR title and commit msg [Improvement][Style] add autolink to issues in PR title and commit msg Feb 6, 2023
@advancedxy
Copy link
Contributor Author

I would suggest omit issue id in the title, and just add it in the PR description.

### Why this change needed?

Fix #123

The point to add issue id in the title is that we can links to issues and commits in Github's ecosystem. In most cases, only title is displayed for commits.

Another benefit to include issue id in PR title(included in commit title automatically) is the commit is also linked to the issue.
See #545 for example

image

@kaijchen
Copy link
Contributor

kaijchen commented Feb 6, 2023

The point to add issue id in the title is that we can links to issues and commits in Github's ecosystem. In most cases, only title is displayed for commits.

My point is, by just looking at the issue id, you don't get any information.
When looking at commit history, you got interested by the scope and type of the change first.
Then you start to looking for why it's needed, and find the related issue.

Just click the 3 dots, you could see it the description.

image

@advancedxy
Copy link
Contributor Author

The point to add issue id in the title is that we can links to issues and commits in Github's ecosystem. In most cases, only title is displayed for commits.

My point is, by just looking at the issue id, you don't get any information.

No, the auto-linked issues is clickable. You can jump to the issue page by simply looking at the title just as you can jump to the pr page.
image

@jerqi
Copy link
Contributor

jerqi commented Feb 8, 2023

It's ok for me.

@advancedxy
Copy link
Contributor Author

image

@kaijchen @zuston does this demonstrate the benefits of autolinks?

@advancedxy
Copy link
Contributor Author

kaijchen zuston jerqi felixcheung WDYT?

also cc @xianjingfeng which was pointed to a wrong one.

@zuston
Copy link
Member

zuston commented Feb 8, 2023

It's OK for me, thanks for your effort. @advancedxy

@xianjingfeng
Copy link
Member

kaijchen zuston jerqi felixcheung WDYT?

also cc @xianjingfeng which was pointed to a wrong one.

It's OK for me.

@kaijchen
Copy link
Contributor

kaijchen commented Feb 8, 2023

Seems the following format is preferred now?

[#123] feat: add xxx feature
[#124] fix: exception in xxx 

@advancedxy
Copy link
Contributor Author

Seems the following format is preferred now?

[#123] feat: add xxx feature
[#124] fix: exception in xxx 

It's preferred to use conventional commits. But I don't think all the committers are ready for this. Let's keep it as a personal preference for a while? If everyone agrees with conventional commits, we can also enforces this.

@advancedxy
Copy link
Contributor Author

Also FYI, I add an exception rule:
image

@kaijchen
Copy link
Contributor

kaijchen commented Feb 8, 2023

How about combine issue ID and semantic commit message ?

[<issue>] <type>(<scope>): <subject>

For example:

[#123] feat(operator): support xxx
[MINOR] refactor: fix typo in variable name
[#233] fix: check null before xxx
[MINOR] chore(deps): bump grpc to xxx

@kaijchen
Copy link
Contributor

Hi everyone, I've edited existing open PR titles (to show examples to new PR). How do you think?

screenshot

kaijchen added a commit that referenced this issue Feb 16, 2023
### What changes were proposed in this pull request?

Update flaky test template and pr template.

### Why are the changes needed?

1. Flaky test template
    * Title is not consistent with other templates.
    * Default parent issue is invalid. 
2. PR template
    * Make the instructions shorter to read.
    * Let PR author remove the instructions so they will read (hopefully).
    * Update instructions, and add example for PR title. (Resolves #551)

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

No need.
xianjingfeng pushed a commit to xianjingfeng/incubator-uniffle that referenced this issue Apr 5, 2023
…pache#588)

### What changes were proposed in this pull request?

Update flaky test template and pr template.

### Why are the changes needed?

1. Flaky test template
    * Title is not consistent with other templates.
    * Default parent issue is invalid. 
2. PR template
    * Make the instructions shorter to read.
    * Let PR author remove the instructions so they will read (hopefully).
    * Update instructions, and add example for PR title. (Resolves apache#551)

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

No need.
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 a pull request may close this issue.

5 participants