Skip to content

Comments

Cleanup bstream based on post-merge feedback received on 7390#7413

Merged
codesome merged 3 commits intoprometheus:masterfrom
pracucci:cleanup-bstream
Jun 18, 2020
Merged

Cleanup bstream based on post-merge feedback received on 7390#7413
codesome merged 3 commits intoprometheus:masterfrom
pracucci:cleanup-bstream

Conversation

@pracucci
Copy link
Contributor

@codesome left some valuable feedback to #7390 after getting it merged. In this PR I'm addressing the feedback.

We also agreed offline to not address this comment because it's hard to know whether the Fast() version of a function is expected to return an error or not for every single call.

pracucci added 2 commits June 18, 2020 09:03
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

Thanks! Will merge on green

@roidelapluie
Copy link
Member

Where is the copyright going then?

@roidelapluie
Copy link
Member

"Redistributions of source code must retain the above copyright notice"

@pracucci
Copy link
Contributor Author

Where is the copyright going then?

It was erroneously copied from bstream.go but it doesn't apply to the test file. The test file should just have the standard Prometheus license.

@roidelapluie
Copy link
Member

For bstream.go, to respect Redistributions in binary form must reproduce the above copyright notice, it should be added in the NOTICE file.

@codesome
Copy link
Member

The code in bstream_test.go does not copy any code from there, so I don't think it's necessary

@roidelapluie
Copy link
Member

for _test, ok but bstream.go sounds like something that should be added to NOTICE for the binary redistribution

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@codesome
Copy link
Member

Let's fix that in another PR

@codesome codesome merged commit 3b529dd into prometheus:master Jun 18, 2020
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice 👍

@pracucci pracucci deleted the cleanup-bstream branch June 22, 2020 09:50
@roidelapluie roidelapluie mentioned this pull request Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants