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

Feedback #1

Open
sindresorhus opened this issue Oct 1, 2019 · 8 comments
Open

Feedback #1

sindresorhus opened this issue Oct 1, 2019 · 8 comments
Labels
question Further information is requested

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 1, 2019

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 sindresorhus added the question Further information is requested label Oct 1, 2019
@mattt
Copy link

mattt commented Oct 7, 2019

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:

@shengchl
Copy link

shengchl commented Oct 8, 2019

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.

@mattt
Copy link

mattt commented Oct 8, 2019

@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.

@shengchl
Copy link

shengchl commented Oct 9, 2019

@mattt thanks for an in-depth answer!

@roberthein
Copy link

roberthein commented Dec 19, 2019

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?

@sindresorhus
Copy link
Owner Author

Thanks for this valuable feedback @mattt 🙌

  • > I think Percent should be spelled Percentage (see grammarphobia.com/blog/2013/10/percent-vs-percentage.html)
    Fixed: 76480b7
  • > Percent should conform to Numeric (or at the very least, AdditiveArithmetic)
    Fixed: 6ad1e1b
  • > However, it has incorrect implementations for Multiplication and Division:
    Fixed: 50a5144
  • > The current implementation of CustomStringConvertible is unlocalized.
    Fixed: 8720f10
  • > 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: 0513895
  • > 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
Copy link
Owner Author

sindresorhus commented Feb 21, 2020

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:

// XCTAssertEqual((40% + 93%) * 3, 399%)

Any ideas?

@sindresorhus
Copy link
Owner Author

sindresorhus commented Feb 21, 2020

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants