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

Implement Data.Reflectable #289

Merged
merged 8 commits into from
Mar 17, 2022
Merged

Conversation

purefunctor
Copy link
Member

@purefunctor purefunctor commented Mar 16, 2022

Closes #286. Adds a Data.Reflectable module which is where the compiler looks at for the automatically-solved Reflectable type class.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

Could you add tests for both the Reflectable and Reifiable type classes?

@purefunctor
Copy link
Member Author

purefunctor commented Mar 16, 2022

Should I copy-paste the tests from the compiler? https://github.com/purescript/purescript/blob/1d5c618054bc0c452128e8758dc01b758a3a6f39/tests/purs/passing/SolvingReflectable.purs

I think testing Reflectable here would be a bit redundant given the compiler tests. Similarly, tests for IsSymbol is done there too. I'm fine with either way, though 😅

@JordanMartinez
Copy link
Contributor

Should I copy-paste the tests from the compiler? https://github.com/purescript/purescript/blob/1d5c618054bc0c452128e8758dc01b758a3a6f39/tests/purs/passing/SolvingReflectable.purs

I think testing Reflectable here would be a bit redundant given the compiler tests. Similarly, tests for IsSymbol is done there too. I'm fine with either way, though 😅

I think adding it here forces other backends to also implement the tests, which aren't being tested in the compiler. We'd also probably want something more granular.

testReflection :: AlmostEff
testReflection = do
  assert "Symbol reflection" $ reflectType (Proxy :: Proxy "foo") == "foo"
  assert "Int reflection - 1" $ reflectType (Proxy :: Proxy 1) == 1
  assert "Int reflection - 0" $ reflectType (Proxy :: Proxy 0) == 0
  assert "Int reflection - (-1)" $ reflectType (Proxy :: Proxy (-1)) == (-1)
  assert "Boolean - True reflection" $ reflectType (Proxy :: Proxy PB.True) == true
  assert "Boolean - False reflection" $ reflectType (Proxy :: Proxy PB.False) == false
  assert "Ordering - LT reflection" $ reflectType (Proxy :: Proxy PO.LT) == LT
  assert "Ordering - EQ reflection" $ reflectType (Proxy :: Proxy PO.EQ) == EQ
  assert "Ordering - GT reflection" $ reflectType (Proxy :: Proxy PO.GT) == GT

Similarly for the Reifiable which isn't being tested by anything, right?

@JordanMartinez
Copy link
Contributor

🏓 @thomashoneyman

Copy link
Member

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

👍 thanks, @purefunctor!

@thomashoneyman thomashoneyman merged commit 5f1ba9f into purescript:master Mar 17, 2022
@purefunctor purefunctor deleted the reflectable branch March 17, 2022 03:09
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.

Add Data.Reflectable
3 participants