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

Add implicit ofAny from cast to Future. Remove Futuristic #87

Conversation

gene-pavlovsky
Copy link
Member

What do you think about this?
Adds a "wildcard" implicit cast to Future, similar to Promise's "wildcard" ofData<T>(d:T):Promise<T> cast.

@back2dos
Copy link
Member

back2dos commented Jul 3, 2018

In theory, it's the right idea and the implementation is correct. In practice wild card casts are a bit of a minefield, as evidenced here: https://github.com/gene-pavlovsky/tink_core/blob/f85e6a090a94158868ff1ce110be8b2e83904a08/src/tink/core/Future.hx#L81

Will check what this does to various projects. If @kevinresol and @benmerckx can confirm that it's fine, then we can gladly merge this.

@kevinresol
Copy link
Member

kevinresol commented Jul 4, 2018

I compared the generated code. I see now that returned futures are wrapped in ofFutureObject everwhere. But I think it will work.

image

@gene-pavlovsky
Copy link
Member Author

@kevinresol maybe it's not so nice to have that extra generated code.
An alternative is to remove the ofFutureObject and use any of the other workarounds here: https://try.haxe.org/#e7C09
What do you think?

@kevinresol
Copy link
Member

@gene-pavlovsky I think it is fine. Because normal user shouldn't need to cast from FutureObject

@gene-pavlovsky
Copy link
Member Author

@kevinresol I've fixed the conflicts? Are you ok to merge this?

@gene-pavlovsky gene-pavlovsky force-pushed the future-merge-futuristic-into-future branch 2 times, most recently from 83ccaf8 to ee402f6 Compare September 20, 2018 13:58
…ristic-into-future

# Conflicts:
#	tests/Futures.hx
@gene-pavlovsky gene-pavlovsky force-pushed the future-merge-futuristic-into-future branch from ee402f6 to a2f04e9 Compare September 20, 2018 13:59
@kevinresol
Copy link
Member

I think we agreed to remove ofFutureObject?

@gene-pavlovsky
Copy link
Member Author

It's been a while, I forgot about this :)

@kevinresol
Copy link
Member

Tried on my project and the diff looks good. @benmerckx do you have any comments?

@benmerckx
Copy link
Member

@kevinresol No issues here

@gene-pavlovsky gene-pavlovsky merged commit 753c3e8 into haxetink:master Sep 24, 2018
@gene-pavlovsky gene-pavlovsky deleted the future-merge-futuristic-into-future branch September 24, 2018 10:03
@back2dos
Copy link
Member

Just as a note if anyone else runs into this: as it turns out, this kinda backfired for Haxe 3. All the static extensions for Future<T> now get applied to any T because of HaxeFoundation/haxe#5924

So for example if you have using tink.state.Promised; using tink.CoreApi; then tink.core.Future.or shadows tink.state.Promised.PromisedTools.or. Flipping the usings fixes it, but it's not exactly pretty. Anyway, I think we'd best leave it as is, as this shouldn't affect too many people (i.e. those who use tink_state and PromisedTools in particular and are still on Haxe 3).

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.

4 participants