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

Migrate to dhall 1.26.1 and merge types and terms #54

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

clementd-fretlink
Copy link
Contributor

@clementd-fretlink clementd-fretlink commented Oct 2, 2019

The latest dhall version allows to have types and terms in a given record. That allows to use records as proper modules without having separate files for types and terms.

Since all the files are small here, I merged related types and terms. I think it improves clarity (by improving locality)

Things to look at

I've loosely followed the conventions used in Prelude (https://github.com/dhall-lang/dhall-lang/tree/master/Prelude):

  • a package.dhall file per dir
  • when a module exports both an eponymous type and values, the type is exported under Type

What I did not follow:

  • for Addon and Config, the type is defined inline instead of in a separate file. The multiple files containing a single line made navigating our codeconfigbase tedious, so improving locality is a net improvement imo
  • all files have a .dhall extension (omitting it would break our tooling or would require brittle config rules)

ToDo

I'd like to test an end-to-end

@clementd-fretlink clementd-fretlink changed the title Migrate to dhall 1.26.1 and merge types and terms [WIP] Migrate to dhall 1.26.1 and merge types and terms Oct 2, 2019
Copy link

@cyrilfretlink cyrilfretlink left a comment

Choose a reason for hiding this comment

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

Shouldn’t exports.dhall be named package.dhall to comply with the convention in Dhall prelude?

@clementd-fretlink
Copy link
Contributor Author

ah great point, I wasn't aware of this convention. I'll make the change

@clementd-fretlink
Copy link
Contributor Author

(if there are other conventions I'm not following, please tell me)

@clementd-fretlink clementd-fretlink force-pushed the polykinds branch 2 times, most recently from 8f51cc1 to 0505d02 Compare October 2, 2019 09:23
dhall/Config Outdated Show resolved Hide resolved
dhall/Config Outdated Show resolved Hide resolved
Copy link
Contributor

@paulrbr-fl paulrbr-fl left a comment

Choose a reason for hiding this comment

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

I don't really like files without extensions (it can be confusing for new people coming on the dhall codebase) would you mind keeping all dhall files with the .dhall extension?

@clementd-fretlink
Copy link
Contributor Author

« je vois que nous en sommes arrivés à la même conclusion, avec cependant un léger avantage pour moi : j'ai remis les .dhall » (extremely OSS117 voice)

Copy link
Contributor

@paulrbr-fl paulrbr-fl left a comment

Choose a reason for hiding this comment

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

Looks good, I'll approve but please try to do an end-to-end test before merging :)

Copy link

@ismaelbouyaf ismaelbouyaf left a comment

Choose a reason for hiding this comment

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

That’s a nice change!

@clementd-fretlink
Copy link
Contributor Author

Yeah, I'm quite happy with it :-)

Copy link
Contributor

@paulrbr-fl paulrbr-fl left a comment

Choose a reason for hiding this comment

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

I will happily approve now that an end-to-end test was performed (and succeeded)

@clementd-fretlink clementd-fretlink changed the title [WIP] Migrate to dhall 1.26.1 and merge types and terms Migrate to dhall 1.26.1 and merge types and terms Oct 7, 2019
@clementd-fretlink clementd-fretlink merged commit 78c6d04 into fretlink:master Oct 7, 2019
@clementd-fretlink clementd-fretlink deleted the polykinds branch October 7, 2019 07:55
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.

4 participants