Skip to content

make the type of extract richer #1011

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

Merged
merged 12 commits into from
Jun 24, 2019

Conversation

divarvel
Copy link
Contributor

@divarvel divarvel commented Jun 17, 2019

See #387 for the whole context. The main idea is to make extract return a more detailed errors (currently, it returns a Maybe).

Following discussion with @ocharles, and based on @sellout's comments in the issue, I went with a Validation (NonEmpty ExtractError), where ExtractError is either InvalidType or a freeform text for user-supplied errors. Reading the tests, the example provided by @sellout seems at odds with the contact that extract should only fail on type errors.

For now it does not track line number or anything else, but thanks to the applicative instance, it accumulates errors for lists and records. Instead of providing a monad instance through a newtype, I added a validate function which is essentially >>=.

I've added dependencies to either (for Validation) and to semigroupoids (for Data.Functor.Alt, as NonEmptyList a only forms a semigroup).

ToDo

  • add a proper error message for the errors that are not type mismatches
  • make tests pass (there is an error message mismatch)

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

Looks great so far! Just a few small suggestions

@divarvel
Copy link
Contributor Author

divarvel commented Jun 19, 2019

As suggested, i've special-cased the error message for a single error (so that it doesn't change current behaviour). I've added a small message when there are multiple errors, and added an error message for extraction errors.

Once we've found a satisfying solution with validate, I can squash my commits.
I'll also need to put proper bounds on the new dependencies, but I'm not sure how. I've used stackage for too long now

Copy link
Collaborator

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

It looks like the stack-lts6.yaml's extra-deps section needs newer versions of some dependencies to support the either/semigroupoids dependencies

@divarvel
Copy link
Contributor Author

I can drop semigroupoids and use ealt instead of <!>.

@divarvel divarvel force-pushed the richer-extract-errors branch from b2f391f to 52a48e2 Compare June 20, 2019 08:19
@divarvel
Copy link
Contributor Author

I've downgraded deps in stack-lts-6.yaml, but there are still issues with transformers-compat.
I've tried removing semigroupoids as well, but it still fails. I'm not sure what I can try next.

@Gabriella439
Copy link
Collaborator

@divarvel: I'll take a look at it tonight to see if I can get it building with stack's LTS6 resolver

@divarvel
Copy link
Contributor Author

Thanks!

@Gabriella439
Copy link
Collaborator

Alright, I have a pull request up against your branch that I believe should fix the LTS 6 build failures: divarvel#1

@divarvel divarvel force-pushed the richer-extract-errors branch from cf74352 to 4453bf5 Compare June 24, 2019 08:32
@divarvel
Copy link
Contributor Author

divarvel commented Jun 24, 2019

Thanks a lot! It looks like it's fixed on LTS-6. I've rebased on latest master, so unless something else is missing, I can squash the commits.

@Gabriella439
Copy link
Collaborator

@divarvel: You're welcome! There's no need to rebase or squash ahead of time because the repository settings enforce that we "Squash and merge" anyway when merging pull requests

@Gabriella439 Gabriella439 merged commit b9aeb34 into dhall-lang:master Jun 24, 2019
@divarvel divarvel changed the title [WIP] make the type of extract richer make the type of extract richer Jun 24, 2019
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.

3 participants