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

Add Natural/showHex builtin #1351

Closed
wants to merge 1 commit into from
Closed

Conversation

kukimik
Copy link
Contributor

@kukimik kukimik commented Sep 30, 2023

Closes #1336.

Matching Haskell implementation: dhall-lang/dhall-haskell#2538.

This was mostly copy-pasted from 74233ca, sometimes without much understanding.

I decided for uppercase letters A-F (for no particular reason) and adding the 0x prefix (so that the resulting strings are valid Dhall code). I'm not attached to it and this can be easily modified. On the other hand, changing uppercase to lowercase or removing/replacing the prefix is easy with Text/replace, so this decision does not have much weight: utilities for custom formatting can be implemented as Dhall functions.

  • standard/beta-normalization.md probably needs some corrections. What I came up with doesn't look good to me. Probably it could be done better with a comment. I would be grateful for suggestions.

@kukimik
Copy link
Contributor Author

kukimik commented Oct 18, 2023

I don't know what to do about the error:

> ./scripts/lint-prelude.sh 

error: builder for '/nix/store/v4g0vw6li99nlipb614cb1cli2d8b982-expected-prelude.drv' failed with exit code 1;
       last 10 log lines:
       >
       > Error: Unbound variable: Natural/showHex
       >
       > 7│       Natural/showHex
       >
       > /nix/store/a79dlrpjyg4nfa895hckz348lf09p8zf-expected-prelude/Natural/showHex.dhall:7:7
       >
       > 3│   ./showHex.dhall
       >
       > /nix/store/a79dlrpjyg4nfa895hckz348lf09p8zf-expected-prelude/Natural/showHex:3:3
       For full logs, run 'nix log /nix/store/v4g0vw6li99nlipb614cb1cli2d8b982-expected-prelude.drv'.

Perhaps this comment:

I'll also help you tie the knot by merging the corresponding Haskell PR so that this will pass the dhall-lang CI

applies here.

@kukimik kukimik marked this pull request as ready for review October 18, 2023 20:30
@kukimik
Copy link
Contributor Author

kukimik commented Oct 21, 2023

I started to doubt whether this is a good addition to the language. It only solves a very specific problem. My current feelings are that it would be better to add some Natural arithmetic operations (see #1213), which would allow us to easily and efficiently implement showHex in Dhall. I'm not closing the PR yet - I'd love to hear an opinion of someone more involved with the language (@Gabriella439?).

@Gabriella439
Copy link
Contributor

Sorry for the delay on reviewing this

Regarding implementing this in the language, I think the most promising approach to adding a safe division operator is this one (in that same issue you linked):

#1213 (comment)

@kukimik
Copy link
Contributor Author

kukimik commented Nov 4, 2023

Sorry for the delay on reviewing this

Don't worry, there is no rush. 🙂 Thanks for your reply!

Regarding implementing this in the language, I think the most promising approach to adding a safe division operator is this one (in that same issue you linked):

#1213 (comment)

I'll look into this when time permits. I close this PR and the related dhall-haskell PR for now.

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.

Natural -> Hex Builtin
2 participants