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

Strings: support using NSNumber as variable in stringsdict #777

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

KhaosT
Copy link

@KhaosT KhaosT commented Sep 21, 2020

  • I've started my branch from the develop branch (gitflow)
    • And I've selected develop as the "base" branch for this Pull Request I'm about to create
  • I've added an entry in the CHANGELOG.md file to explain my changes and credit myself
    (or added #trivial to the PR title if I think it doesn't warrant a mention in the CHANGELOG)
    • Add the entry in the appropriate section (Bug Fix / New Feature / Breaking Change / Internal Change)
    • Add a period and 2 spaces at the end of your short entry description
    • Add links to your GitHub profile to credit yourself and to the related PR and issue after your description

This PR added the ability to use %@ in NSStringFormatValueTypeKey for the stringsdict file. The generated Swift interface will take in NSNumber which allows localized string to be formatted more accurately when dealing with floating number with high precisions.

I wonder if this is something that this project is interested in merging?

@SwiftGen-Eve
Copy link

SwiftGen-Eve commented Sep 21, 2020

Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!


Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

@djbe
Copy link
Member

djbe commented Oct 2, 2020

Does Localizable.strings(dict) support NSNumber? Can't see that anywhere in the docs. Have you tested this in a working project? (without SwiftGen)
What's the deal with %ℝ?

@KhaosT
Copy link
Author

KhaosT commented Oct 2, 2020

Yeah, stringsdict supports NSNumber.

Simulator Screen Shot - iPhone 11 - 2020-10-02 at 10 51 50

override func viewDidLoad() {
        super.viewDidLoad()

        textLabel.text = String(format: NSLocalizedString("plural.objects", comment: ""), NSNumber(1))
        textLabel2.text = String(format: NSLocalizedString("plural.objects", comment: ""), NSNumber(10))
}

With the stringsdict file look like:

<dict>
	<key>plural.objects</key>
	<dict>
		<key>NSStringLocalizedFormatKey</key>
		<string>%#@object_num@</string>
		<key>object_num</key>
		<dict>
			<key>NSStringFormatSpecTypeKey</key>
			<string>NSStringPluralRuleType</string>
			<key>NSStringFormatValueTypeKey</key>
			<string>@</string>
			<key>one</key>
			<string>one object</string>
			<key>other</key>
			<string>%@ objects</string>
		</dict>
	</dict>
</dict>

@KhaosT
Copy link
Author

KhaosT commented Oct 2, 2020

Since the rest of the SwiftGen is operating based on string format specifier, to avoid huge structural changes and maintain support for normal .strings file (with %@ as normal object), I picked %ℝ as a special substitution for %@ when parsing stringsdict file for numbers. %ℝ is selected because it's unique and unlikely to accidentally collide with other real string format specifiers.

@djbe
Copy link
Member

djbe commented Oct 2, 2020

Aha, all right, if it works, we probably should support it, cumbersome as it might be 😅

Quick question: does your comment of "allows more precision" actually work though? AFAIK stringsdict only supports rules for "one", "zero", "two", "few", "many" and "other".

Regarding the actual implementation, I'll try to take a look at it later. I understand the trick you did with ℝ, but I'm hoping I can find something simple specific for the stringsdict side of parsing without it.

@KhaosT
Copy link
Author

KhaosT commented Oct 2, 2020

Allows more precision is more around the issue with formatting floating point number after arithmetic operations. By allowing NSNumber, we can actually use NSDecimalNumber in these location for formatting, avoiding this issue compared to dealing with other primitive type.

@djbe
Copy link
Member

djbe commented Oct 3, 2020

NSDecimalNumber

Shouldn't that be Decimal then?

Regardless of that, in general programmers should never pass a Float/Double/... directly into a user-visible String, it should always go through a formatter like NumberFormatter, to correctly format according to locale + have control over other formatting aspects.

Which gets back to my initial point: why pass real numbers to something that expects countable numbers? AFAIK stringsdict can only make decisions based on the whole part of a number. Allowing NSNumber (or Decimal) would give the programmer the illusion it's going to work for actual floating point numbers, and that it's an OK thing to do (it isn't).

@KhaosT
Copy link
Author

KhaosT commented Oct 3, 2020

Since %@ expects an Objective-C object, NSNumber (and related subclasses) are used in this case.

I agree that in general, user facing numbers should be formatted explicitly to string for the optimal experience. However, given NSLocalizedString already supports %f and %d for Float and Int, I'm not sure there is a difference allowing support for NSNumber given the underlying system actually supports that? And it actually works with floating point numbers, like NSNumber(3.5) will correctly be identified as some category and use that rule for the actual formatting.

There are situations where the call site cares more about showing the fractional value (like quantity, weight, time) accurately with the correct unit that doesn't necessarily bother with formatting numbers super nicely (like 1 minute / 2.5 minutes, 1 kg / 6.18 kgs). Providing support for NSNumber allows this category of usage.

@djbe
Copy link
Member

djbe commented Oct 3, 2020

Since %@ expects an Objective-C object, NSNumber (and related subclasses) are used in this case.

If we add this, in the Swift templates we should generate something that accepts Decimal. In the Obj-C templates we should generate code with NSDecimalNumber.

given NSLocalizedString already supports %f and %d for Float and Int

Yes. they do. In the case of Int, it's not a big issue, but Float generally won't do what a user expects. But we're talking about stringsdict, which works on the "countable set", so integers only (or the whole part of floating points I think).

like 1 minute / 2.5 minutes, 1 kg / 6.18 kgs

Bad examples, for those cases people should use DateComponentsFormatter and MeasurementFormatter respectively.

You're glossing over the fact that what'll actually happen is that people will try something like 1.02 and end up with "1.01999999999 apples", where they would expect "1,02 apples" (limit decimals, use correct separator). Again, yes I know they can make the same mistake with %f, but stringsdict plurals will give people the illusion they're on the right path.

@KhaosT
Copy link
Author

KhaosT commented Oct 3, 2020

Since %@ expects an Objective-C object, NSNumber (and related subclasses) are used in this case.

If we add this, in the Swift templates we should generate something that accepts Decimal. In the Obj-C templates we should generate code with NSDecimalNumber.

Yeah make sense. Originally I thought the NumberFormatter only has a NumberFormatter.string(from:) which takes in NSNumber so the API for localized string would match this and by doing so, the use case I had in mind will require less type conversion from NSDecimalNumber to Decimal.

Turns out there is a different variant from the superclass NumberFormatter.string(for:) which works with Decimal, so while having a signature for NSNumber is still preferable for my case, I'm cool going with Decimal.

given NSLocalizedString already supports %f and %d for Float and Int

Yes. they do. In the case of Int, it's not a big issue, but Float generally won't do what a user expects. But we're talking about stringsdict, which works on the "countable set", so integers only (or the whole part of floating points I think).

like 1 minute / 2.5 minutes, 1 kg / 6.18 kgs

Bad examples, for those cases people should use DateComponentsFormatter and MeasurementFormatter respectively.

You're glossing over the fact that what'll actually happen is that people will try something like 1.02 and end up with "1.01999999999 apples", where they would expect "1,02 apples" (limit decimals, use correct separator). Again, yes I know they can make the same mistake with %f, but stringsdict plurals will give people the illusion they're on the right path.

For this, I guess I'm a little unsure when exactly should stringsdict plurals be used directly then? Given it doesn't format integer with desired separator (for %lu, %d, etc), it almost should never be used to format any numbers that's user facing directly even if they are countable... I'm not sure we can encourage a usage like below, given we can't control how people write their stringsdict 😅

String(
    format: NSLocalizedString("files.copied", comment: ""),
    1000,
    numberFormatter.string(for: 1000)!
)

@djbe djbe changed the title Support using NSNumber as variable for stringsdict Strings: support using NSNumber as variable in stringsdict Oct 4, 2020
@marcelofabri
Copy link
Member

@djbe anything I can do to help moving this forward? while I agree that in most cases it makes sense to use formatters, I do think this can be helpful. Maybe putting it behind a flag would make it less invasive?

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.

None yet

4 participants