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

🌱 Simplify Code-Review #1584

Closed
wants to merge 2 commits into from
Closed

🌱 Simplify Code-Review #1584

wants to merge 2 commits into from

Conversation

azeemshaikh38
Copy link
Contributor

  • Please check if the PR fulfills these requirements

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Refactor Code-Review to simplify it. No behavior change.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No.

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 2, 2022 01:25 Inactive
@azeemshaikh38 azeemshaikh38 marked this pull request as draft February 2, 2022 01:28
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[b45c160]
(https://github.com/ossf/scorecard/actions/runs/1781646891)

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 2, 2022 01:36 Inactive
@azeemshaikh38 azeemshaikh38 marked this pull request as ready for review February 2, 2022 01:37
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[9b4b98a]
(https://github.com/ossf/scorecard/actions/runs/1781678101)

laurentsimon
laurentsimon reviewed Feb 2, 2022
View reviewed changes
Copy link
Contributor

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

I'm in the middle of submitting an earlier PR on separating check/policy for this check. Can the simplification wait till I've merged?

@azeemshaikh38
Copy link
Contributor Author

I'm in the middle of submitting an earlier PR on separating check/policy for this check. Can the simplification wait till I've merged?

It might actually be easier for you to rebase your PR on top of #1566 and this one. With these changes, Code-Review raw data would be equivalent to returning ListCommits API data as is. Let me know.

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 2, 2022 15:11 Inactive
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[46eeb1f]
(https://github.com/ossf/scorecard/actions/runs/1784601673)

@azeemshaikh38 azeemshaikh38 temporarily deployed to integration-test February 2, 2022 16:38 Inactive
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Integration tests success for
[02a485f]
(https://github.com/ossf/scorecard/actions/runs/1785037858)

@laurentsimon
Copy link
Contributor

I'm in the middle of submitting an earlier PR on separating check/policy for this check. Can the simplification wait till I've merged?

It might actually be easier for you to rebase your PR on top of #1566 and this one. With these changes, Code-Review raw data would be equivalent to returning ListCommits API data as is. Let me know.

I'd rather wait to land it. There is not much to change once my PR lands. But If we land this PR, I have to re-factor the code again. I've already re-factored it 3 times ^^

@azeemshaikh38 azeemshaikh38 force-pushed the azeems/commit branch 4 times, most recently from a58ab56 to c0a8c8d Compare February 2, 2022 23:37
Base automatically changed from azeems/commit to main February 3, 2022 00:01
@azeemshaikh38 azeemshaikh38 deleted the azeems/code-review branch August 29, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurentsimon laurentsimon laurentsimon left review comments

@naveensrinivasan naveensrinivasan Awaiting requested review from naveensrinivasan

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

Successfully merging this pull request may close these issues.
3 participants
@azeemshaikh38 @laurentsimon @azeemsgoogle