Skip to content

Conversation

@kbendick
Copy link
Contributor

@kbendick kbendick commented Jan 9, 2021

I came across this small a nit of a formatting issue when working on another ticket.

This aligns the comments for all of the metrics instantiated when building the metrics of the TestDataFile.

@kbendick kbendick changed the title Nit - Align all comment's indentation in TestStrictMetricsEvaluator Nit - Align all comment indentation in TestStrictMetricsEvaluator Jan 9, 2021
@github-actions github-actions bot added the API label Jan 9, 2021
@kbendick
Copy link
Contributor Author

This came up while working on this issue: #1952

.put(13, 1L)
.build(),
// nan value counts
// nan value counts
Copy link
Contributor Author

@kbendick kbendick Jan 10, 2021

Choose a reason for hiding this comment

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

There are multiple maps being built above and below this that have a comment like this, but this comment doesn't align with the others.

Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

Thanks for noticing this!

@rdblue
Copy link
Contributor

rdblue commented Jan 11, 2021

I appreciate taking the time to keep the code clean, but I generally like to avoid commits that could cause conflicts simply to fix whitespace. It isn't unclear what the comment applies to, so I don't think that we should make this change.

@jackye1995
Copy link
Contributor

I appreciate taking the time to keep the code clean, but I generally like to avoid commits that could cause conflicts simply to fix whitespace. It isn't unclear what the comment applies to, so I don't think that we should make this change.

@yyanyy can you include this change in one of your commits? I think you added that comment line

@yyanyy
Copy link
Contributor

yyanyy commented Jan 12, 2021

I appreciate taking the time to keep the code clean, but I generally like to avoid commits that could cause conflicts simply to fix whitespace. It isn't unclear what the comment applies to, so I don't think that we should make this change.

@yyanyy can you include this change in one of your commits? I think you added that comment line

Yes, sorry I introduced this. I think currently I don't have any PR that touches this file though, and I think #2062 will likely modify this class; should we do it there instead so that we won't have conflict when either PR got merged?

@kbendick
Copy link
Contributor Author

I appreciate taking the time to keep the code clean, but I generally like to avoid commits that could cause conflicts simply to fix whitespace. It isn't unclear what the comment applies to, so I don't think that we should make this change.

That makes sense to me. I can update the formatting in #2062 if we'd like.

@kbendick
Copy link
Contributor Author

I appreciate taking the time to keep the code clean, but I generally like to avoid commits that could cause conflicts simply to fix whitespace. It isn't unclear what the comment applies to, so I don't think that we should make this change.

That makes sense to me. I can update the formatting in #2062 if we'd like.

I pushed this change in #2062 as it already touches this file, so I'm closing this PR.

@kbendick kbendick closed this Jan 13, 2021
@kbendick kbendick deleted the formatting-nit-in-teststrictmetricsevaluator branch January 13, 2021 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants