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

[#2196] Use git blame line info for aggregate blame author modified and date info #2232

Merged
merged 34 commits into from
Jan 6, 2025
Merged

[#2196] Use git blame line info for aggregate blame author modified and date info #2232

merged 34 commits into from
Jan 6, 2025

Conversation

logical-1985516
Copy link
Contributor

@logical-1985516 logical-1985516 commented Jul 3, 2024 •
edited
Loading

Fixes #2196

Proposed commit message

Uses blameLine to abstract away the string wrangling and improve
understandability. Magic numbers are also replaced to improve code
quality.

In order to use blameLine however, it is also changed to use 
author-time instead of commit-time, as there is a discrepancy
between the two, causing some test cases to fail.

The naming of the timestamp field in GitBlameLineInfo is changed
to reflect that it is in seconds, as author-time is in seconds.

Other information

Surrounding code which states milliseconds is also changed to state seconds correctly, as this PR is meant to resolve a code quality issue anyways. If this is not ok, I can revert the last commit.

@ckcherry23 ckcherry23 requested a review from a team July 3, 2024 16:54
@logical-1985516 logical-1985516 changed the title 2196 use git blame line info for aggregate blame author modified and date info [#2196] Use git blame line info for aggregate blame author modified and date info Jul 4, 2024
@ckcherry23 ckcherry23 mentioned this pull request Jul 6, 2024
Closed
@logical-1985516
Copy link
Contributor Author

logical-1985516 commented Jul 7, 2024 •
edited
Loading

Hi, I have noticed that some test cases take a very long time to finish. Could this be due to the additional overhead incurred from running git blame command in blameLine?

@logical-1985516
Copy link
Contributor Author

Hi, would anyone like to approve the workflow? I am not sure why the local test cases run indefinitely, so I am unable to verify that it passes all test cases. Thank you!

@damithc
Copy link
Collaborator

damithc commented Jul 15, 2024

Hi, would anyone like to approve the workflow? I am not sure why the local test cases run indefinitely, so I am unable to verify that it passes all test cases. Thank you!

done

@logical-1985516
Copy link
Contributor Author

Hi, would anyone like to help me for this issue? I could not seem to figure why the test cases are failing. More information at this discussion.

gok99
gok99 requested changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@gok99 gok99 left a comment

Choose a reason for hiding this comment

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

Really sorry for the review delays! I've left some clarifications about the intent of the original issue. Let me know if you've got questions.

@ckcherry23 ckcherry23 requested a review from gok99 August 3, 2024 01:38
@gok99 gok99 removed the Stale label Sep 21, 2024
@gok99
Copy link
Contributor

gok99 commented Sep 21, 2024

@logical-1985516 Are you planning to wrap up this PR and #2238? I think this PR is mostly good to merge and is just pending some updates to tests.

@logical-1985516
Copy link
Contributor Author

Hi @gok99, for this PR, I may be able to wrap up this PR within next week, if I have the time.
However, I probably would not wrap up PR #2238. Sorry about that.

@gok99
Copy link
Contributor

gok99 commented Oct 24, 2024

@logical-1985516 thanks for the fixes. made a minor name change.

@github-actions GitHub Actions
Copy link
Contributor

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

@github-actions github-actions bot added the Stale label Nov 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 7, 2024

This PR was closed because it has been marked as stale for 7 days with no activity.
Feel free to reopen this PR if you would like to continue working on it.

@github-actions github-actions bot closed this Dec 7, 2024
@gok99 gok99 reopened this Dec 7, 2024
@gok99 gok99 removed the Stale label Dec 7, 2024
jonasongg
jonasongg approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

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

LGTM!

georgetayqy
georgetayqy approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@georgetayqy georgetayqy left a comment

Choose a reason for hiding this comment

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

LGTM!

Though, we should look into upgrading the macOS runners since macOS 12 runners are being deprecated and GitHub Actions is not running the tests for macOS 12.

@georgetayqy georgetayqy mentioned this pull request Dec 24, 2024
Open
@gok99 gok99 merged commit e53c52c into reposense:master Jan 6, 2025
15 of 16 checks passed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 6, 2025

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonasongg jonasongg jonasongg approved these changes

@georgetayqy georgetayqy georgetayqy approved these changes

@gok99 gok99 gok99 approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use GitBlameLineInfo for processing git blame output in aggregateBlameAuthorModifiedAndDateInfo
6 participants
@logical-1985516 @damithc @gok99 @georgetayqy @jonasongg @ckcherry23