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

fix ifElse typings #162

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tusharmath
Copy link

No description provided.

@KiaraGrouwstra
Copy link
Member

Thanks, looks like I missed the extra params. On the first one you missed the [] though. Also, since it seems these param sets are connected, would you mind capturing them into that generic after all as an array (e.g that T)? You'll want to connect this to the input params as well (all the way at the end).

@tusharmath
Copy link
Author

@tycho01

that generic after all as an array (e.g that T)

I don't think that's applicable to ifElse, consider the following valid usage of ifElse

const temp = ifElse(
  R.nthArg(0),
  R.nthArg(1),
  R.nthArg(2)
)

temp(true, 'Apples', 1000)

Its not possible to capture such arguments — boolean, string and number in a generic AFAIK. Or may be I din't understand your comment completely?

@KiaraGrouwstra
Copy link
Member

KiaraGrouwstra commented Mar 28, 2017

Sorry, you're right, unfortunately the spread fails on generics. Otherwise I thought <T extends any[], U, V> should be possible. But in that case:

  • the remaining T should probably go as well, you left one without it being declared anymore.
  • perhaps we can use both your definition and the only unary one that did allow binding them using generics.
    Alternatively, I supposed we could have separate versions with 1/2/3/... params such as to capture everything (most accurate, though it could go on for a while...).

@tusharmath
Copy link
Author

tusharmath commented Mar 29, 2017

@tycho01 would it really be help to write multiple versions of typings with 1, 2, 3 or 4 params? IMHO its alright to use any[] :-)

@KiaraGrouwstra
Copy link
Member

I wouldn't ask you to, but that said, such an overloading strategy is what'd create for the typings with the most powerful inference, actually allowing to check whether the params you provided match up with what the functions want. I'd overlooked the option to use 2+ params, but otherwise that was the intent of the generic in the original.

@tusharmath
Copy link
Author

tusharmath commented Mar 30, 2017

I think that's fair. We need to also consider that these arguments are going to be curried. Probably handle that too?

PS: Why is the build failing?

@KiaraGrouwstra
Copy link
Member

KiaraGrouwstra commented Mar 30, 2017

Right, there's currying too. Feel free to either handle or leave it for now. Given that the number of combinations kind of explodes, I've been intending to handle that using code generation, see here. But yeah, I don't mind taking care of that later.

P.S.: on the build, I'm aware of it; not all tests are passing yet, some due to issues with TypeScript itself.

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.

2 participants