Skip to content

add eitherRelAbs #32

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

Closed
wants to merge 1 commit into from
Closed

Conversation

safareli
Copy link
Contributor

also express maybeRel maybeAbs using eitherRelAbs

@safareli safareli requested review from garyb and cryogenian December 21, 2017 16:11
also express maybeRel maybeAbs using eitherRelAbs
@garyb
Copy link
Member

garyb commented Dec 21, 2017

This seems a bit questionable, as it should already be known from the types whether a path is Abs or Rel, should it not?

@cryogenian
Copy link
Member

@garyb Not really e.g. forall s. Path s a b. We can do following

class AbsOrRel s where 
  absOrRel :: forall a b s. Path s a b -> Either (Path Rel a b) (Path Abs a b)

Do you think it would be better?

@garyb
Copy link
Member

garyb commented Dec 21, 2017

I'm just not sure how we'd end up with a path value that already isn't Rel or Abs in the types? It makes sense that there can be functions that operate on Path with polymorphic Abs/Rel, I just don't know how we'd actually have one that has it unspecified.

@safareli
Copy link
Contributor Author

@garyb good point I went all they up and at some point the information was lost i.e. forall x was used instead of the Path.Rel so the use case I had for it is not relevant any more.

But I the I like the idea of having one function which returns either vs two which return maybe. As Path is Either Rel or Abs and the function proposed describes that statement in more clear form then maybes.

@garyb
Copy link
Member

garyb commented Dec 22, 2017

But I the I like the idea of having one function which returns either vs two which return maybe

I agree with that, but I don't think we need maybeRel / maybeAbs either 😄 (aside from an internal implementation thing perhaps).

@safareli
Copy link
Contributor Author

will be fixed as part of #33

@safareli safareli closed this Feb 13, 2018
@safareli safareli deleted the rel-abs branch February 13, 2018 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants