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

dhall-toml: add support for lists #2231

Merged
merged 6 commits into from
Jul 2, 2021
Merged

dhall-toml: add support for lists #2231

merged 6 commits into from
Jul 2, 2021

Conversation

ear7h
Copy link
Collaborator

@ear7h ear7h commented Jun 26, 2021

TOML lists are bit tricky they require some special care, in part because the old version of the spec required that they are all the same type. Also, here are links to issues I mention in the comments:

@ear7h ear7h requested review from Gabriella439 and sjakobi June 26, 2021 08:27
Gabriella439
Gabriella439 reviewed Jun 27, 2021
View reviewed changes
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
@ear7h
Copy link
Collaborator Author

ear7h commented Jun 28, 2021

@Gabriel439 Took a bit of refactoring, but I removed all the NonEmpty related partial functions. I think it looks a bit messy but it's more explicit wrt the idiosyncrasies of TOML.

sjakobi
sjakobi reviewed Jun 28, 2021
View reviewed changes
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Just a few minor wibbles.

dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Show resolved Hide resolved
dhall-toml/src/Dhall/Toml.hs Outdated Show resolved Hide resolved
@Gabriella439 Gabriella439 merged commit 1477514 into master Jul 2, 2021
@Gabriella439 Gabriella439 deleted the toml branch July 2, 2021 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjakobi sjakobi sjakobi left review comments

@Gabriella439 Gabriella439 Gabriella439 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.
3 participants
@ear7h @Gabriella439 @sjakobi