Percentage icon indicating copy to clipboard operation
Percentage copied to clipboard

Feedback

Open sindresorhus opened this issue 5 years ago • 8 comments

Is this a good idea? Any downsides with such a type? Would it make sense to propose adding this kind of type to Swift itself?

sindresorhus avatar Oct 01 '19 06:10 sindresorhus

Thanks for sharing this library with the community, @sindresorhus. I wanted to share some feedback that I hope might be helpful / informative:

Is this a good idea? Any downsides with such a type?

I think percentages can be a helpful abstraction in some circumstances, but I've yet to see any implementation that was complete, correct, and widely supported (the closest I've seen is in CSS, and even still there are some inconsistencies in when percentages are a valid unit). Unless this were part of the Swift standard library, I think adoption of this type would do more harm than good compared to using Double.

Would it make sense to propose adding this kind of type to Swift itself?

Percentages are a particular kind of Rational numbers, where the denominator is equal to 100. With this framing, it's a lot clearer what the requirements for such a type are.

Currently, Precent fulfills 5 of its arithmetic properties correctly:

(50%) == (50%) // true (Equality)
(5%) < (50%) // true (Comparison)
(50%) + (50%) // 100% (Addition)
(50%) - (50%) // 0% (Subtraction)
-(50%) // -50% (Negation)

However, it has incorrect implementations for Multiplication and Division:

(50%) * (50%) // 2500% (should be 25%)
(50%) / (50%) // 1% (should be 100%)

As far as how this would be received on the Swift forums, I'd direct you to previous threads proposing formal mathematical types, like Real and Complex. I think it'd be an uphill battle to convince folks that Percent is something that should be included in the standard library, and would likely have to be part of the larger, aforementioned formalization (which I don't think is likely to happen anytime soon).


Some additional feedback:

  • I think Percent should be spelled Percentage (see https://www.grammarphobia.com/blog/2013/10/percent-vs-percentage.html)
  • Percent should conform to Numeric (or at the very least, AdditiveArithmetic)
  • The current implementation of CustomStringConvertible is unlocalized. Percentages have different representations across different locales (see https://github.com/Flight-School/Guide-to-Swift-Numbers-Sample-Code/blob/master/Chapter%202/Percentages.playground/Contents.swift)
  • I think rawValue should be spelled differently, although I don't have a strong suggestion what that should be (my weak preference is numerator, but that would entail adding a denominator property for balance).
  • There should be a way to support interoperation with CGFloat without importing the CoreGraphics framework and manually providing overloads. See FloatingPoint and related protocols.
  • The % operator should be usable for all built-in ExpressibleByIntegerLiteral types (most notably, Float)

mattt avatar Oct 07 '19 17:10 mattt

Hey there, @mattt

I have little understanding of the subject but just out of the curiosity would like to ask your opinion on whether SwiftUI's Angle is a an abstraction similar to Percent? If so, we may see it adopted in the future versions of the framework sooner rather than later. I think having Percent would be very useful in a UI framework.

shengchl avatar Oct 08 '19 18:10 shengchl

@shengchalover That’s a great question. I think angles are different from percentages in several important ways:

  • Angle is a scalar + unit, whereas percentage is just a scalar
  • Angles are commonly converted back and forth between degrees and radians
  • Angles are expressed in base 360 and pi, whereas percentages are merely a ratio to 100, in familiar base 10

For these reasons, I think it makes sense to have an Angle type, whereas it’s be difficult to make the case for a Percentage type.

mattt avatar Oct 08 '19 19:10 mattt

@mattt thanks for an in-depth answer!

shengchl avatar Oct 09 '19 20:10 shengchl

I like this idea, I'm using something similar to use % in swift:

postfix operator %
postfix public func %<T: FloatingPoint>(lhs: T) -> T { lhs / 100 }

Maybe not as fancy as your Percent class, but I can use % everywhere I want. And in combination with the angle ° operator:

postfix operator °
postfix public func °<T: FloatingPoint>(lhs: T) -> T { lhs * ((.pi * 2) / 360) }

I can do something like:

let x: Float = 50% * -180° // -1.5707964

wdyt?

roberthein avatar Dec 19 '19 11:12 roberthein

Thanks for this valuable feedback @Mattt 🙌

  • [x] > I think Percent should be spelled Percentage (see grammarphobia.com/blog/2013/10/percent-vs-percentage.html) Fixed: https://github.com/sindresorhus/Percentage/commit/76480b7625dbcd9074babefc248d87e336f13071
  • [x] > Percent should conform to Numeric (or at the very least, AdditiveArithmetic) Fixed: https://github.com/sindresorhus/Percentage/commit/6ad1e1bf2ea1ba23a2a5e122e89832d266225ef1
  • [x] > However, it has incorrect implementations for Multiplication and Division: Fixed: https://github.com/sindresorhus/Percentage/commit/50a5144702f03239c5ebc5d8a652b9bb8536ff5a
  • [x] > The current implementation of CustomStringConvertible is unlocalized. Fixed: https://github.com/sindresorhus/Percentage/commit/8720f1002f82abe81f427a26669c090b84021975
  • [x] > There should be a way to support interoperation with CGFloat without importing the CoreGraphics framework and manually providing overloads. See FloatingPoint and related protocols. Fixed: https://github.com/sindresorhus/Percentage/commit/0513895234bdf42f451e52211a552e47800326af
  • [ ] > The % operator should be usable for all built-in ExpressibleByIntegerLiteral types (most notably, Float)
  • [ ] > think rawValue should be spelled differently, although I don't have a strong suggestion what that should be (my weak preference is numerator, but that would entail adding a denominator property for balance).

sindresorhus avatar Feb 21 '20 14:02 sindresorhus

One problem I'm having is that because Percentage conforms to ExpressibleByIntegerLiteral, it will interpret the 2 in 10% * 2 as a Percentage, not a Double, which means the result will not be correct. For example, this test is currently failing: https://github.com/sindresorhus/Percentage/blob/50a5144702f03239c5ebc5d8a652b9bb8536ff5a/Tests/PercentageTests/PercentageTests.swift#L10

Any ideas?

sindresorhus avatar Feb 21 '20 14:02 sindresorhus

The % operator should be usable for all built-in ExpressibleByIntegerLiteral types (most notably, Float)

I assume you mean that this should work?

let float: Float = 5%

think rawValue should be spelled differently, although I don't have a strong suggestion what that should be (my weak preference is numerator, but that would entail adding a denominator property for balance).

No strong opinion on this, and not sure why it matters.

Percent should conform to Numeric

Shouldn't it conform to BinaryFloatingPoint instead?

sindresorhus avatar Feb 21 '20 14:02 sindresorhus