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

Provide multiple files to various CLI subcommands #2169

Merged
merged 13 commits into from
Apr 23, 2021

Conversation

basile-henry
Copy link
Collaborator

This PR makes it possible to provide multiple files as argument to the format, freeze, and lint subcommands.
These subcommands already had to deal with potentially multiple files via the use of the transitive flag.

Motivation

It is common in unix tools for the CLI to accept multiple file arguments as input, so it would be great if dhall could do the same where possible/it makes sense.
A more concrete motivation is that I would like dhall format to be easily usable with formatting tools/managers such as https://github.com/numtide/treefmt. treefmt has a specification of sorts for what basic API formatters should expose as part of their CLI: https://numtide.github.io/treefmt/docs/formatters-spec.html. This specification requires the ability for a formatter to take multiple file arguments.

Providing multiple files as argument as opposed to calling these subcommands in a loop in bash (or with xargs -n1) has some performance benefits since we don't have to start a new process for every file.

Changes

This PR make inplace implicit for file arguments (so that flag is removed). That was already kind of the case for these 3 subcommands as the only way to provide a file argument was inplace or transitive which implied inplace.

This PR changed the transitive flag from a flag that takes a file path to a switch that applies to all the input files.

Stdin is still supported, in 2 ways now: no file arguments, or a file argument "-". This should enable applying the subcommands to both stdin and input files at the same time.
Note: stdin is still a non-transitive input even when the transitive switch is on.


Other subcommands might benefit from taking multiple files as input as well, I simply started with the ones I thought needed it the most.

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! Just a few comments

dhall/src/Dhall/Util.hs Outdated Show resolved Hide resolved
dhall/src/Dhall/Util.hs Outdated Show resolved Hide resolved
dhall/src/Dhall/Main.hs Outdated Show resolved Hide resolved
dhall/src/Dhall/Util.hs Outdated Show resolved Hide resolved
@basile-henry
Copy link
Collaborator Author

Thanks for the review @Gabriel439 😄

Copy link
Collaborator

@TristanCacqueray TristanCacqueray left a comment

Choose a reason for hiding this comment

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

The change seems to work as expected, thanks! Would it be ok to keep a deprecated --inplace argument to ease the transition to avoid breaking CI?

@basile-henry
Copy link
Collaborator Author

Would it be ok to keep a deprecated --inplace argument to ease the transition

I am already breaking backwards compatibility with --transitive in this PR by making it a switch instead of an flag that takes an argument. So I'm not sure keeping a backward compatible --inplace (with argument I assume) makes that much sense. In general I think it's sensible to make breaking API changes in one go.

to avoid breaking CI

In this PR? I thought everything passed at the moment, or are there other tests that are only run on master that I need to update?

I still need to have a closer look at documentation to make sure the CLI flags are up to date there as well.

I should also add a changelog entry for this change. 😄

@TristanCacqueray
Copy link
Collaborator

I agree it's better to make breaking API changes in one go. Though this one is likely to affect integration jobs, e.g. https://github.com/search?q=%22dhall+format+--inplace%22&type=Code .

@Gabriella439
Copy link
Collaborator

One thing that can smooth the transition is to make --inplace a switch that does nothing but emits a warning (so that old usages of dhall format --inplace someFile.dhall still work during the migration period)

@TristanCacqueray
Copy link
Collaborator

If the switch does nothing, then it might be better to make it fail though. Note that this will also break the vim plugin according to https://github.com/vmchale/dhall-vim/blob/master/ftplugin/dhall.vim#L32

@basile-henry
Copy link
Collaborator Author

I have added --inplace back as a switch this time. It doesn't do anything other than outputting a notice of deprecation on stderr.
It is also hidden from --help which should hopefully discourage further use.

We can revert commit 2d6035a at a later time if when we want to remove the flag entirely.

@basile-henry
Copy link
Collaborator Author

It looks like dhall-lang will need to be updated after this change as well: https://github.com/dhall-lang/dhall-lang/search?q=%22--inplace%22

But at least with this deprecation approach we shouldn't break too many workflows/scripts. Do you think something similar is needed for --transitive? I expect that to be used much less often

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.

Sorry, I missed that you moved this out of Draft status. This looks great!

@basile-henry
Copy link
Collaborator Author

No worries, I don't work on this as often as I'd like, so slow PR progress works for me. As long as it's not blocking others.

But I think it's ready now, so I will merge it, and hopefully not too many people will have issues with the new CLI API 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants