-
Notifications
You must be signed in to change notification settings - Fork 9
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
Typecheck fully resolved imports #24
Conversation
ab780ca
to
823171e
Compare
with ResolvingInput | ||
class AlphaNormalizationSuite(val base: String) extends ExprOperationAcceptanceSuite(_.alphaNormalize) with ParsingInput | ||
class NormalizationSuite(val base: String) extends ExprOperationAcceptanceSuite(_.normalize) with CachedResolvingInput | ||
class NormalizationUSuite(val base: String) extends ExprOperationAcceptanceSuite(_.normalize) with ParsingInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the U
suffix here. What does it mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means I wanted to call it Unit
but that name was already taken 😂
@@ -25,6 +25,9 @@ class TypeCheckingSimpleSuite extends CachingTypeCheckingSuite("type-inference/s | |||
class TypeCheckingUnitSuite extends CachingTypeCheckingSuite("type-inference/success/unit") | |||
class TypeCheckingRegressionSuite extends TypeCheckingSuite("type-inference/success/regression") | |||
class TypeCheckingOtherSuite extends TypeCheckingSuite("type-inference/success") { | |||
//TODO prelude is WAY too slow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's way too slow? To be honest I'd rather hold off on this change for now and keep it easy to make sure we're correctly type-checking the unnormalized prelude in CI, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prelude test is what was hanging and aborting the CI builds. We can do, it just means we don't conform to the spec and will have to disable one or two other tests and I thought most of this was covered in the Prelude
suites anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's go ahead and merge this after the Prelude
name change revert and I'll take a closer look later at making sure we can run this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'm going to have a look into the Haskell implementation to see if it does any optimization (caching type-checking results, etc)
class HashingPreludeBoolOddSuite extends HashingSuite("semantic-hash/success/prelude/Bool/odd") | ||
class HashingPreludeBoolOrSuite extends HashingSuite("semantic-hash/success/prelude/Bool/or") | ||
class HashingPreludeBoolShowSuite extends HashingSuite("semantic-hash/success/prelude/Bool/show") | ||
class HashingBoolAndSuite extends HashingSuite("semantic-hash/success/prelude/Bool/and") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to keep it easy to distinguish the prelude tests from others (both in the results and with testOnly
). I'm not necessarily against removing the Prelude
from the test class name, but if we do I think these should go in a new org.dhallj.tests.acceptance.prelude
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry!!! That was an unintentional sed
change. Will revert
@TimWSpence Thanks! Will merge on green. |
No description provided.