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

Fix DataSize class according to ISO/IEC 80000 #23682

Conversation

newzubakhin
Copy link

This PR fixes DataSize class according to ISO/IEC 8000 spec (see Binary prefix)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 23, 2019
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Sep 24, 2019
@sbrannen
Copy link
Member

At a glance, this looks like a series of breaking changes in the API that has existed since Spring Framework 5.1, and such breaking changes are not acceptable.

If we were to add ISO 8000 support, it would have to be "in addition" to the existing feature set.

@newzubakhin
Copy link
Author

Ok, I'll implement something like ISODataSize class with a corresponding converter and tests.
Should I close this PR?

@snicoll
Copy link
Member

snicoll commented Sep 24, 2019

@newzubakhin please hold on. As Sam indicated, the current proposal is a breaking change and we won't and can't change DataSize that way. Before you spend more time on the topic, I'd like to get a chance to review the suggestion and the impacts of having two separate DataSize types.

@snicoll
Copy link
Member

snicoll commented Sep 25, 2019

Thanks for the PR @newzubakhin. We've discussed this and decided to keep DataSize in its current form and clarify its Javadoc, see #23697

@snicoll snicoll closed this Sep 25, 2019
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed in: core Issues in core modules (aop, beans, core, context, expression) status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 25, 2019
@sstock
Copy link

sstock commented Apr 9, 2021

I understand keeping the existing DataSize behavior for backward compatibility. However I encourage adding a standards compliant variation as suggested by @newzubakhin. The #23697 documentation certainly helps with direct consumers of the class. But does not alleviate the need to replicate such clarification to values parsed by DataSize, e.g. Spring Boot externalized configuration. The current behavior creates confusion as The International System of Units (9th edition, section 3 Decimal multiples and sub-multiples of SI units) states "The SI prefixes refer strictly to powers of 10. They should not be used to indicate powers of 2 (for example, one kilobit represents 1000 bits and not 1024 bits)." This wording is mirrored by NIST's SI prefixes summary.

@vpavic
Copy link
Contributor

vpavic commented May 25, 2022

As this was declined due to being a breaking change back in 2019, could you reconsider it for 6.0?

IMO it's very unfortunate to have a piece of javadoc that basically redefines something as well established as the SI prefixes. If there's a configuration property of type DataSize annotated with @DataSizeUnit(DataUnit.KILOBYTES) that should be quite self explanatory.

@vbezhenar
Copy link

My suggestion is to support both forms, like DataUnit.KILOBYTES, DataUnit.KIBIBYTES, etc. KILOBYTES will not change from current meaning, but @Deprecated probably should be added, so people would avoid it. And support parsing "KiB" suffix as well.

So basically does not break backwards compatibility but adds support for proper units.

For me it was surprising to find out this terminology used in Spring. I think at this point all technical usage should adhere to ISO.

@vpavic
Copy link
Contributor

vpavic commented Oct 19, 2022

As someone who got burned by this in a project that surfaced DataSize through configuration properties, I'd kindly ask the team again to reconsider this. Not respecting (and redefining) the SI prefixes only has the potential to cause confusion, and I can only echo what @sstock already pointed out:

The #23697 documentation certainly helps with direct consumers of the class. But does not alleviate the need to replicate such clarification to values parsed by DataSize, e.g. Spring Boot externalized configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants