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

[WIP]: #3682, add metrics #3743

Closed
wants to merge 4 commits into from
Closed

Conversation

GoelJatin
Copy link

Issue: #3682

Creating this PR to address the first requirement listed in the issue.

Will gradually work on the other requirements as well, and waiting for more details from the issue owner on this one, to address it completely.

CC: @Madhu94 , @yuvipanda

@minrk
Copy link
Member

minrk commented Aug 1, 2018

Hi @GoelJatin, thanks for the PR! It looks like an autoformatter has been run on this code. Can you please revert these changes and rebase without them? It is very hard to review a pull request that has a large amount of these changes, since we can't find which changes are to the behavior and which are to the style. Once those are removed, we can get started reviewing the changes here.

An excellent rule of thumb when contributing to projects in general: If running an auto-formatter on the code produces a large change, then the project probably does not use the autoformatter in question and would likely not welcome the changes the autoformatter would produce. Please avoid 'cleanup' pull requests in general unless they have been explicitly requested.

@GoelJatin
Copy link
Author

Hi @minrk ,

Sure I understand your point.

Will remove the commits for formatting, and update the PR.

Thanks for the advice!

@blink1073
Copy link
Contributor

Closing this PR as stale as part of housekeeping, please feel free to re-open if you'd like to continue working on it @GoelJatin.

@blink1073 blink1073 closed this Jun 8, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants