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.

Sign up for GitHub

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

Jump to bottom

test(git-authors): add unit test #1098

Merged
merged 2 commits into from
Nov 18, 2023
Merged

test(git-authors): add unit test #1098

merged 2 commits into from
Nov 18, 2023

Conversation

vanpipy
Copy link
Collaborator

@vanpipy vanpipy commented Oct 31, 2023 •
edited
Loading

Close #1088

@vanpipy vanpipy force-pushed the feature/git-authors branch from fb1139f to 795d048 Compare October 31, 2023 10:55
hyperupcall
hyperupcall reviewed Oct 31, 2023
View reviewed changes
bin/git-authors Outdated Show resolved Hide resolved
@vanpipy vanpipy force-pushed the feature/git-authors branch from 78721e2 to 0759c4a Compare October 31, 2023 13:28
@vanpipy vanpipy changed the title test(git-authors): add unit test and refactor it test(git-authors): add unit test Nov 1, 2023
spacewander
spacewander reviewed Nov 3, 2023
View reviewed changes
Copy link
Collaborator

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Let's update https://github.com/tj/git-extras/blob/main/Commands.md#git-authors too

man/git-authors.1 Show resolved Hide resolved
@vanpipy vanpipy force-pushed the feature/git-authors branch 2 times, most recently from 2f49eba to 55ae8ab Compare November 3, 2023 07:34
@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 3, 2023

Let's update https://github.com/tj/git-extras/blob/main/Commands.md#git-authors too

Update

@vanpipy vanpipy force-pushed the feature/git-authors branch from 55ae8ab to 4809d1f Compare November 4, 2023 13:37
hyperupcall
hyperupcall approved these changes Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

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

LGTM, I just had a comment and a question about the code.

spacewander
spacewander reviewed Nov 11, 2023
View reviewed changes
Copy link
Collaborator

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Would you split this PR into the break change & the test part so I can merge it?

@vanpipy vanpipy force-pushed the feature/git-authors branch from 4809d1f to 43b6134 Compare November 11, 2023 09:09
@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 11, 2023 •
edited
Loading

Would you split this PR into the break change & the test part so I can merge it?

Update and I set the BREAKING CHANGE in the description. Do I need to update it into the commit message type?

@hyperupcall
Copy link
Collaborator

@vanpipy By adding the explanation mark feat(git-authors)!:, I don't think you need to - there should be a dedicated BREAKING CHANGES section in the changelog unless I am mistaken.

But you still should combine the other test-tested commits into a single one. Remember, I recommended there be two. If you don't do this, then the CHANGELOG will be cluttered with entries that are not relevant.

@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 12, 2023 •
edited
Loading

@vanpipy By adding the explanation mark feat(git-authors)!:, I don't think you need to - there should be a dedicated BREAKING CHANGES section in the changelog unless I am mistaken.

But you still should combine the other test-tested commits into a single one. Remember, I recommended there be two. If you don't do this, then the CHANGELOG will be cluttered with entries that are not relevant.

Alright, let me figure out,

  1. Do I need split this PR to two? No, I do not understand ths point cause I split the changes and its testcases to two commits. @spacewander @hyperupcall
  2. combine the changes and its testcases into one commit? Yes, I get that.

The thing here is,

  1. create a breaking change commit
  2. create its testcases commit
  3. create other configuration change commit

Is it ok? Or I got something misataken?

@vanpipy vanpipy force-pushed the feature/git-authors branch from 43b6134 to d68e3c4 Compare November 12, 2023 08:37
@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 12, 2023 •
edited
Loading

I follow the history behavior to update the breaking change information onto the commit type. Please review this later.

@spacewander
Copy link
Collaborator

@vanpipy By adding the explanation mark feat(git-authors)!:, I don't think you need to - there should be a dedicated BREAKING CHANGES section in the changelog unless I am mistaken.
But you still should combine the other test-tested commits into a single one. Remember, I recommended there be two. If you don't do this, then the CHANGELOG will be cluttered with entries that are not relevant.

Alright, let me figure out,

  1. Do I need split this PR to two? No, I do not understand ths point cause I split the changes and its testcases to two commits. @spacewander @hyperupcall
  2. combine the changes and its testcases into one commit? Yes, I get that.

The thing here is,

  1. create a breaking change commit
  2. create its testcases commit
  3. create other configuration change commit

Is it ok? Or I got something misataken?

The benefit to split the test case out of break change is that we can port the either test case or the break change to different versions.

Since git-extras doesn't have multiple versions, it's OK to accept current behavior if you insist on it.

@spacewander spacewander self-requested a review November 13, 2023 03:43
@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 13, 2023

Since git-extras doesn't have multiple versions, it's OK to accept current behavior if you insist on it.

In fact is that I did not know the version strategy of git-extras for now 😢 . I just follow the conventional commit...

If you can clarify it - a document is great, I am happy to follow it.

@hyperupcall
Copy link
Collaborator

@vanpipy For the future, just for clarification, what I was describing was a commit history that looked like this. Nothing too crazy, just something that I thought would look a little cleaner

@vanpipy vanpipy force-pushed the feature/git-authors branch from d68e3c4 to 083d3f0 Compare November 14, 2023 05:41
@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 14, 2023

@vanpipy For the future, just for clarification, what I was describing was a commit history that looked like this. Nothing too crazy, just something that I thought would look a little cleaner

I see, update.

@vanpipy vanpipy force-pushed the feature/git-authors branch from 083d3f0 to a59128f Compare November 14, 2023 05:44
@vanpipy
fix: correct issues with testing helper and config
d4fd47f 
    * chore: add faulthandler_timeout to avoid the process hangs on without any information
    * fix: teardown the named directory cause the process stucks when lots of the files in the named directory
    * refactor: log more for locating directory and remove the invoking command function
@vanpipy
feat(git-authors)!: no longer to support open the editor
0b16d58 
    * BREAKING CHANGE: remove behavior of opening authors with the default editor
    * test: add unit test
@vanpipy vanpipy force-pushed the feature/git-authors branch from a59128f to 0b16d58 Compare November 14, 2023 05:45
@spacewander
Copy link
Collaborator

@vanpipy For the future, just for clarification, what I was describing was a commit history that looked like this. Nothing too crazy, just something that I thought would look a little cleaner

I see, update.

@vanpipy
Could you avoid using force-push on the reviewing PR the next time? It's hard to figure what has been updated in the force-push.

@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 15, 2023

Could you avoid using force-push on the reviewing PR the next time? It's hard to figure what has been updated in the force-push.

I am curious about how to manage the commits of the changes in the PR except the force push. Could you give some advices?

@vanpipy vanpipy requested a review from hyperupcall November 17, 2023 14:46
@hyperupcall
Copy link
Collaborator

hyperupcall commented Nov 17, 2023 •
edited
Loading

I am curious about how to manage the commits of the changes in the PR except the force push. Could you give some advices?

It's impossible to change the commits in the PR without force pushing. That's kind of why I said for the future

@spacewander spacewander merged commit cf9fe2d into tj:main Nov 18, 2023
5 checks passed
@spacewander
Copy link
Collaborator

@vanpipy
Thanks for your contribution!

@spacewander
Copy link
Collaborator

I have submitted a pull request guide at #1113, would you help to review it?

@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 18, 2023

I have submitted a pull request guide at #1113, would you help to review it?

Great, I want to attach this PR as an example to show a bad case, hhahaa...

@vanpipy
Copy link
Collaborator Author

vanpipy commented Nov 18, 2023 •
edited
Loading

I am curious about how to manage the commits of the changes in the PR except the force push. Could you give some advices?

It's impossible to change the commits in the PR without force pushing. That's kind of why I said for the future

There is one way to keep the traces that the draft @spacewander called, the draft PR can keep any commit the contributor want so that the traces all here but there is still need one-force-push at least before merged into main branch. One good part is the reviewer and the contributor can know all changes without losing any detail. Especially, the reviewer can follow the workflow of the contributor.

@vanpipy vanpipy deleted the feature/git-authors branch December 26, 2023 10:03
@chenrui333 chenrui333 mentioned this pull request Apr 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyperupcall hyperupcall hyperupcall approved these changes

@spacewander spacewander spacewander approved these changes

Assignees
No one assigned
Labels
break change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test(git-authors): add unit test
3 participants
@vanpipy @hyperupcall @spacewander