Consider adding --inplace option #40

New Issue

2017-08-05T14:25:23+02:00

fmthoma commented

2017-08-05 14:25:23 +02:00

(Migrated from github.com)

Usage:

brittany --inplace **/*.hs

And since input files can already be specified as positional arguments, the -i parameter could be reassigned as shortcut for --inplace...

(My current workaround: ls **/*.hs | xargs -I{} brittany -i {} -o {})

Usage: ``` brittany --inplace **/*.hs ``` And since input files can already be specified as positional arguments, the `-i` parameter could be reassigned as shortcut for `--inplace`... (My current workaround: `ls **/*.hs | xargs -I{} brittany -i {} -o {}`)

👍 1

lspitzner commented

2017-08-05 15:50:36 +02:00

(Migrated from github.com)

good point.

thinking aloud a bit:

With your example, brittany would of course need to accept [FILE..] and not just [FILE], so just quickly aliasing --inplace x to --input x --output x is not sufficient. (A great reason for me to refactor the monstrosity that is the effective main function. whistles innocently)
We probably want abort on a per-file basis (syntax error in one file -> all other are still processed).
Might want error/warning output to be prefixed with the current file, or just print "processing $FILE" for each one in order.
Might want parallelization.

good point. thinking aloud a bit: - With your example, brittany would of course need to accept `[FILE..]` and not just `[FILE]`, so just quickly aliasing `--inplace x` to `--input x --output x` is not sufficient. (A great reason for me to refactor [the monstrosity that is the effective `main` function](https://github.com/lspitzner/brittany/blob/master/src-brittany/Main.hs#L130-L255). *whistles innocently*) - We probably want abort on a per-file basis (syntax error in one file -> all other are still processed). - Might want error/warning output to be prefixed with the current file, or just print "processing $FILE" for each one in order. - Might want parallelization.

👍 2

lspitzner commented

2017-10-02 14:35:30 +02:00

(Migrated from github.com)

PR #59 adds an --inplace flag, but due to limitations of the butcher library the brittany --inplace **/*.hs usecase does not work yet. I'll fix that in butcher next.

PR #59 adds an `--inplace` flag, but due to limitations of the `butcher` library the `brittany --inplace **/*.hs` usecase does not work yet. I'll fix that in `butcher` next.

lspitzner commented

2017-10-03 00:20:12 +02:00

(Migrated from github.com)

I have uploaded butcher-1.1.1.0 which has addStringParams.

@d-dorazio you wanna do the final bit to fix this?

I have uploaded `butcher-1.1.1.0` which has `addStringParams`. @d-dorazio you wanna do the final bit to fix this?

d-dorazio commented

2017-10-03 10:56:11 +02:00

(Migrated from github.com)

@lspitzner sure, but I also have a proposal: specifying input files both with -i and as parameters seem a bit verbose to me so I'd remove both -i and -o and provide an interface similar to what other formatters provide like rustfmt. For example we could add --write-mode inplace|display and the files would be specified only as parameters. In particular, the --write-mode allows to eventually support other formats such as diff.

@lspitzner sure, but I also have a proposal: specifying input files both with `-i` and as parameters seem a bit verbose to me so I'd remove both `-i` and `-o` and provide an interface similar to what other formatters provide like [`rustfmt`](https://github.com/rust-lang-nursery/rustfmt#running). For example we could add `--write-mode inplace|display` and the files would be specified only as parameters. In particular, the `--write-mode` allows to eventually support other formats such as `diff`.

👍 1

lspitzner commented

2017-10-03 15:32:50 +02:00

(Migrated from github.com)

@d-dorazio I agree, the current handling of --input vs param(s) can be improved, and write-mode makes sense. I don't see the verbosity argument - it only affects the (commandline) docs, but I have no strong opinion on this.

For clarification:

no	--inputs	--outputs	params	behaviour
0	-	-	-	stdin to stdout
1	-	-	params	inplace or according to `--write-mode`
2	ifile	-	-	ifile to stdout
3	-	ofile	-	stdin to ofile
4	ifile	ofile	-	ifile to ofile
5	ifiles	ofiles	-	if lengths match, act like `zipWithM`
6	ifiles	ofiles	params	?

Strongly in favour of 0 and 1, even if 1 is a breaking change due to implying --inplace. Slightly in favour of keeping support for 2/3/4. I can do without 5 and 6 (i.e. make param(s) and -i/-o incompatible).

There is one other related item: Do we want to support brittany Foo.hs Bar.hs --flagA --flagB ? And if not, warn about interpreting --flagA as a file name? The best solution probably is to stop supporting file names start with a dash and then allow reordering for params as well. I'll have to make another butcher release for that, whoops. That's what I get for not using optparse-applicative :D

@d-dorazio I agree, the current handling of `--input` vs param(s) can be improved, and `write-mode` makes sense. I don't see the verbosity argument - it only affects the (commandline) docs, but I have no strong opinion on this. For clarification: no | --inputs | --outputs | params | behaviour |-|-|-|-|- 0 | \- | \- | \- | stdin to stdout 1 | \- | \- | params | inplace or according to `--write-mode` 2 | ifile | \- | \- | ifile to stdout 3 | \- | ofile | \- | stdin to ofile 4 | ifile | ofile | \- | ifile to ofile 5 | ifiles | ofiles | \- | if lengths match, act like `zipWithM` 6 | ifiles | ofiles | params | ? Strongly in favour of 0 and 1, even if 1 is a breaking change due to implying `--inplace`. Slightly in favour of keeping support for 2/3/4. I can do without 5 and 6 (i.e. make param(s) and `-i`/`-o` incompatible). There is one other related item: Do we want to support `brittany Foo.hs Bar.hs --flagA --flagB` ? And if not, warn about interpreting `--flagA` as a file name? The best solution probably is to stop supporting file names start with a dash and then allow reordering for params as well. I'll have to make another `butcher` release for that, whoops. That's what I get for not using `optparse-applicative` :D

d-dorazio commented

2017-10-03 16:01:59 +02:00

(Migrated from github.com)

@lspitzner 1 and 2 look unintuitive to me, because I see params as basically equivalent to --inputs. I think that an interface like the following would be simpler and wouldn't provide ambiguities:

# stdin to stdout
$ brittany

# overwrite Main.hs
$ brittany --write-mode overwrite Main.hs

# files to stdout
$ brittany Main.hs src/Lib.hs

# overwrite all the files
$ brittany --write-mode overwrite Main.hs src/Lib.hs

It's true that you'd lose the ability to specify different output files than the input. I think it doesn't happen very often and you can always just redirect the output to the custom output.

If you feel strongly about it then I can implement it the other way though.

@lspitzner 1 and 2 look unintuitive to me, because I see `params` as basically equivalent to `--inputs`. I think that an interface like the following would be simpler and wouldn't provide ambiguities: ```shell # stdin to stdout $ brittany # overwrite Main.hs $ brittany --write-mode overwrite Main.hs # files to stdout $ brittany Main.hs src/Lib.hs # overwrite all the files $ brittany --write-mode overwrite Main.hs src/Lib.hs ``` It's true that you'd lose the ability to specify different output files than the input. I think it doesn't happen very often and you can always just redirect the output to the custom output. If you feel strongly about it then I can implement it the other way though.

lspitzner commented

2017-10-03 17:15:17 +02:00

(Migrated from github.com)

@d-dorazio what ambiguities? what you suggest there is farther from the rustfmt behaviour as you make --write-mode display the default; not that i am strictly opposed to this, but it seems to be the only difference between our suggestions.

@d-dorazio what ambiguities? what you suggest there is farther from the `rustfmt` behaviour as you make `--write-mode display` the default; not that i am strictly opposed to this, but it seems to be the only difference between our suggestions.

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#40

Consider adding --inplace option #40

Consider adding `--inplace` option #40