Thomvis / BrightFutures

Write great asynchronous code in Swift using futures and promises
MIT License
1.9k stars 184 forks source link

Conflict with seconds and milliseconds shortcuts #194

Closed hiltonc closed 5 years ago

hiltonc commented 6 years ago

We recently started using BrightFutures in an app, but have found that the seconds and milliseconds properties on Int are interfering with similar methods in our codebase (ours return TimeInterval). These don't seem like an essential feature of the library, would it be reasonable to not export them? I'd be willing to make a pull request if that would be acceptable.

phimage commented 6 years ago

Maybe some people use it. One way to do it is two add a compilation flag, and do two implementation one with public variables, the other only internals

hiltonc commented 6 years ago

Rather than complicate the library, I'd recommend removing them with a non-backwards compatible version change and a short code snippet in the release notes. They're very simple extensions.

phimage commented 6 years ago

With that logic, we coud provide a minimal framework and ask everyone to copy the same piece of code in all their projects, so where is the limit? I found that using it, is convenient with asyncAfter... (like it is done in unit test, so not part of the framework)

Example of discussion about add or not isEven, isOdd in Swift. foundation. https://github.com/apple/swift-evolution/blob/master/proposals/0225-binaryinteger-iseven-isodd-ismultiple.md - https://forums.swift.org/t/se-0225-adding-iseven-isodd-ismultiple-to-binaryinteger/15382/3

(ps: non-backwards compatible version must not occurs in a 7.0.1, but a 7.1.0 now)

hiltonc commented 6 years ago

I would say the limit is: what is the purpose of the library? A utility library will have all manner of useful extensions. I would expect a futures library to provide futures, promises, etc. The Swift framework is going to be a special case because all developers use it.

Another option would be a CocoaPod subspec for useful extensions.

According to semantic versioning, a non-backwards compatible version would be 8.0.0 (increment major version)

phimage commented 6 years ago

But 7.0.0 is out and it will take time to have a new release. So there is also a practical way, a swift way with breaking change every times, even in swift ;(

By the way if you use cocoapod and that could help you, feel free to PR. Default target must keep all the code for everyone, but we could have a Core subspec without some extensions.

Thomvis commented 6 years ago

Thanks for the discussion, @hiltonc and @phimage . I think there's a good argument for removing the second/millisecond helpers. It's not part of the core functionality of the framework.

I remember adding those helpers because I didn't like the way DispatchTime has to be created. Looking at it again, I think it's actually fine. This:

XCTAssert(f.forced(500.milliseconds.fromNow) != nil)

would become:

XCTAssert(f.forced(.now() + .milliseconds(500)) == nil)

I'd be open to fixing this and releasing it soon. The version number would be 8.0.0 indeed. I'm not sure if I have time for it, so a PR is very welcome, @hiltonc. If it is still relevant to you after all this time.

hiltonc commented 5 years ago

Thank you!