Skip to content

Conversation

@carterkozak
Copy link
Contributor

This makes it obvious that the values must be used together.

==COMMIT_MSG==
Replace HumanReadableByteCount getSize/getUnit with more obvious map(func)
==COMMIT_MSG==

Possible downsides?

API churn

Alternatives?

  1. We could return a new tuple type instead of taking a function. I suspect that would have a similar usability problem though.
  2. We could implement an error-prone check similar to JavaDurationGetSecondsGetNano, but I'd prefer to design APIs which needn't be accompanied by static analysis.

This makes it obvious that the values must be used together.
@carterkozak carterkozak requested a review from schlosna June 29, 2021 19:41
@changelog-app
Copy link

changelog-app bot commented Jun 29, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Replace HumanReadableByteCount getSize/getUnit with more obvious map(func)

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from jkozlowski June 29, 2021 19:42
@carterkozak carterkozak changed the title Replace HumanReadableByteCount getSize/getUnit with more obvious map(func)` Replace HumanReadableByteCount getSize/getUnit with more obvious map(func) Jun 29, 2021
*
* @deprecated in favor of {@link #map(SizeFunction)} which is harder to misuse.
*/
@DoNotCall

Choose a reason for hiding this comment

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

Will this fail the build for people? Do we need to write a migration for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it won’t cause transitive failures. The problem with a migration is that it won’t fix incorrect uses.

Copy link

@jkozlowski jkozlowski left a comment

Choose a reason for hiding this comment

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

Happy to approve

@schlosna
Copy link
Contributor

Thanks @carterkozak , this should be more resilient to potential misuse

@bulldozer-bot bulldozer-bot bot merged commit 65a3f96 into develop Jun 30, 2021
@bulldozer-bot bulldozer-bot bot deleted the ckozak/footgun branch June 30, 2021 12:27
@svc-autorelease
Copy link
Collaborator

Released 1.4.0

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants