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.

Sign up for GitHub

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

Jump to bottom

Ports purescript-proxy into this repo #230

Merged
merged 16 commits into from
Oct 10, 2020
Merged

Ports purescript-proxy into this repo #230

merged 16 commits into from
Oct 10, 2020

Conversation

JordanMartinez
Copy link
Contributor

Since SProxy is defined in Prelude, we need to port purescript-proxy's Proxy type to here instead now that it can be polykinded.

Note: I assume that IsSymbol should use Proxy now rather than SProxy.

natefaubion
natefaubion reviewed Oct 5, 2020
View reviewed changes
thomashoneyman
thomashoneyman reviewed Oct 6, 2020
View reviewed changes
@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Oct 6, 2020 •
edited
Loading

I'll have to get to this later:

  • Update all kind-specific proxy types to use forall proxy. proxy kindValue

@thomashoneyman thomashoneyman mentioned this pull request Oct 6, 2020
Closed
hdgarrood
hdgarrood reviewed Oct 6, 2020
View reviewed changes
src/Data/Symbol.purs Outdated

-- local definition for use in `reifySymbol`
foreign import unsafeCoerce :: forall a b. a -> b

reifySymbol :: forall r. String -> (forall sym. IsSymbol sym => SProxy sym -> r) -> r
reifySymbol s f = coerce f { reflectSymbol: \_ -> s } SProxy where
reifySymbol :: forall r. String -> (forall sym. IsSymbol sym => Proxy sym -> r) -> r
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use a type variable proxy instead of Proxy, for the same reason as reflectProxy - that is, so that it works with both Proxy and SProxy until we update it again later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I've fixed that in latest commits.

@JordanMartinez
Copy link
Contributor Author

Should the docs for Proxy be updated at all? I think right now it only explains how to use it as a proxy for kind Type, not for any kinds.

@hdgarrood
Copy link
Contributor

I don't think that's necessary; it will be clear that Proxy can be used for any kind once someone has come across the idea of polymorphic kinds, and I think the documentation for polykinds would be a better place to introduce things like the fact that you can use Proxy with any kind.

@JordanMartinez
Copy link
Contributor Author

Ok. Then I think this PR is ready to be merged if there are no other changes I need to make.

@hdgarrood hdgarrood merged commit 212e486 into master Oct 10, 2020
@hdgarrood hdgarrood deleted the addProxy branch October 10, 2020 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natefaubion natefaubion natefaubion left review comments

@thomashoneyman thomashoneyman thomashoneyman left review comments

@hdgarrood hdgarrood hdgarrood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JordanMartinez @hdgarrood @natefaubion @thomashoneyman