add --inplace flag #59

2017-10-01T17:05:46+02:00

d-dorazio commented

2017-10-01 17:05:46 +02:00

(Migrated from github.com)

This PR adds support for:

passing multiple inputs/outputs via the -i/-o flags;
the --inplace flag as a shortcut for overwriting the input files with the formatted output.

I'm still trying to learn Haskell, so suggestions are appreciated 😄 .

This PR adds support for: - passing multiple inputs/outputs via the `-i`/`-o` flags; - the `--inplace` flag as a shortcut for overwriting the input files with the formatted output. I'm still trying to learn Haskell, so suggestions are appreciated 😄 .

lspitzner commented

2017-10-01 17:39:06 +02:00

(Migrated from github.com)

Heh, i was just working on brittany too. I want to take a break now, but will have a look at this later.

lspitzner (Migrated from github.com) reviewed 2017-10-01 21:02:11 +02:00

lspitzner (Migrated from github.com) left a comment

Looks good! It seems to address all points from #40 apart from the parallelization, which is fine. I have still done some nitpicking :p

Optimally we'd also want to support multiple inputParams, but it seems butcher does allow for that yet.. I should probably fix that.

Looks good! It seems to address all points from #40 apart from the parallelization, which is fine. I have still done some nitpicking :p Optimally we'd also want to support multiple `inputParam`s, but it seems `butcher` does allow for that yet.. I should probably fix that.

src-brittany/Main.hs

													
				@ -0,0 +126,4 @@

				        putStrLn $ "Copyright (C) 2016-2017 Lennart Spitzner"

				        putStrLn $ "There is NO WARRANTY, to the extent permitted by law."

				      System.Exit.exitSuccess

				    when printHelp $ do

lspitzner (Migrated from github.com) commented

2017-10-01 19:18:42 +02:00

Any particular reason not to do Left errNo -> return $ Just ErrNo ?

Any particular reason not to do `Left errNo -> return $ Just ErrNo` ?

lspitzner (Migrated from github.com) commented

2017-10-01 19:34:04 +02:00

foldM sure works, but is overkill. You execute the same thing for each ios element, so i'd say it is more idiomatic to separate the IO and the non-IO: use results <- sequence ios; let result = _ results (or make it a one-liner via fmap).

Btw the reason I used Either e () instead of Maybe e is that I usually interpret Nothing as the failure-case, but it would be inverted here. But you can fold lists of either (Either or Maybe) into one element using sequence_ or asum, respectively.

`foldM` sure works, but is overkill. You execute the same thing for each `ios` element, so i'd say it is more idiomatic to separate the IO and the non-IO: use `results <- sequence ios; let result = _ results` (or make it a one-liner via `fmap`). Btw the reason I used `Either e ()` instead of `Maybe e` is that I usually interpret `Nothing` as the failure-case, but it would be inverted here. But you can fold lists of either (Either or Maybe) into one element using `sequence_` or `asum`, respectively.

lspitzner (Migrated from github.com) commented

2017-10-01 20:53:24 +02:00

In my opinion the case-of that was above before is nicer. This helper obscures the logic by forcing me to jump half a screen down and back up, with little benefit.

There is another source of redundancy I'd rather focus on: The code above effectively checks inplace twice and null outputPaths twice as well. Merging those two makes more sense imo.

In my opinion the case-of that was above before is nicer. This helper obscures the logic by forcing me to jump half a screen down and back up, with little benefit. There is another source of redundancy I'd rather focus on: The code above effectively checks `inplace` twice and `null outputPaths` twice as well. Merging those two makes more sense imo.

lspitzner commented

2017-10-02 14:27:44 +02:00

(Migrated from github.com)

Thanks! It is merged into dev branch.

I'll add a note to #40.

Thanks! It is merged into `dev` branch. I'll add a note to #40.

d-dorazio commented

2017-10-02 14:27:45 +02:00

(Migrated from github.com)

@lspitzner the reason why I don't exit with errNo is that if more files fail then the exit code would depend on the order of the inputs and imo that'd be strange. I can do it if you feel strongly about it.

I should have addressed all the other comments though.

@lspitzner the reason why I don't exit with `errNo` is that if more files fail then the exit code would depend on the order of the inputs and imo that'd be strange. I can do it if you feel strongly about it. I should have addressed all the other comments though.

lspitzner commented

2017-10-02 14:31:25 +02:00

(Migrated from github.com)

I wouldn't have minded depending on the order, but I can understand that you don't like it. In the end you can just not communicate very much in one Int. I have merged as-is.

I wouldn't have minded depending on the order, but I can understand that you don't like it. In the end you can just not communicate very much in one `Int`. I have merged as-is.

👍 1

lspitzner commented

2017-10-02 14:33:22 +02:00

(Migrated from github.com)

Btw by using sequence for the IO part, making things parallel should be a one-liner.

Btw by using sequence for the `IO` part, making things parallel should be a one-liner.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

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#59