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

call dhall with "format" argument #13

Closed
wants to merge 1 commit into from

Conversation

justinwoo
Copy link
Contributor

Set the dhall format subcommand to match the new Dhall arguments

Fix #12

@sellout
Copy link
Contributor

sellout commented Jun 20, 2018

Is it worth keeping any backward compatibility here?

I’d also like to change the type header to use dhall type instead of dhall 2>, so this question applies there as well.

dhall-mode.el Outdated
(apply 'call-process dhall-format-command nil errbuf t (append dhall-format-options (list
(buffer-file-name))))
(apply 'call-process dhall-command nil errbuf t (append dhall-format-options (list
(buffer-file-name))))
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should explicitly (cons "format" dhall-format-options) rather than adding "format" to the customizable options.

@psibi
Copy link
Owner

psibi commented Jun 20, 2018

@sellout Thanks for the review! It would be nice to keep backward compatibility as I myself use an older version yet.

@sellout
Copy link
Contributor

sellout commented Jun 20, 2018

The frustrating bit is that new versions require dhall version and older ones require dhall --version … so it‘s not like we can easily get the answer currently. We need to either just see which command works and dispatch on “new” vs “old” or something like

(or (run-command "dhall version") (run-command "dhall --version"))

and then we can dispatch on the actual version.

Whichever we use, should be defined on its own, so we can use it both for the formatting changes here as well as the type header changes.

@justinwoo
Copy link
Contributor Author

I'm okay with keeping this PR open for a while or moving it to some specific branch until there's a new proper version released

@purcell
Copy link
Collaborator

purcell commented Jul 5, 2018

I made a slightly different change relating to this in 8405e7b - for now, if you set dhall-format-command to nil, the code will use dhall format. This allows us to keep backwards compatibility for now. Later, when the most-used (LTS?) dhall version supports the "format" subcommand, we can flip the default.

@joneshf
Copy link

joneshf commented Jul 14, 2018

Can that be added to the documentation? It's not clear from reading the current documentation that setting the value to "dhall format" will not work, nor is it clear that setting the value to nil will work the way it's supposed to.

It took finding this issue, and then finding the above comment to understand how this variable is supposed to be used.

purcell added a commit that referenced this pull request Jul 15, 2018
@purcell
Copy link
Collaborator

purcell commented Jul 15, 2018

Thanks @joneshf - done!

@joneshf
Copy link

joneshf commented Jul 15, 2018

That's wonderful! Thank you so much!

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.

None yet

5 participants