gregwebs / Shelly.hs

Haskell shell scripting
BSD 3-Clause "New" or "Revised" License
418 stars 88 forks source link

Modify shelly to support lifted operations #57

Closed jwiegley closed 10 years ago

jwiegley commented 10 years ago

Hi Greg!

Here is the very first cut. I'm submitting this as a pull request simply to begin a conversation on the best direction(s) for the code to go in. I intentionally have not finished it yet, because at this point I just wanted to prove that the concept works, and to get your feedback.

Here is what I have working now:

What's left to do:

There is only about 3 hours more work left to do, but I wanted you to have a chance to review what I've done so far, to see if the approach is even acceptable to you.

gregwebs commented 10 years ago

Thanks, this looks great! Although I can't say I understand all the control stuff. For testing we could try replacing import Shelly with import Shelly.Lifted in the existing tests.

jwiegley commented 10 years ago

@gregwebs Everything looks done for this first cut. I finished the remaining todo items:

I actually want to use this functionality in a tool I'm writing, where I want to use a ReaderT r Sh a, so porting my code is the next step for me.

It's still unfortunate to have a doubly-nested ReaderT, just to carry around things like command-line option values. But I can't think of anything at the moment which doesn't introduce a lot more complexity. After all, most of the time spent in Shelly will be executing processes, rather than the extra overhead of a nested Reader.

gregwebs commented 10 years ago

After all, most of the time spent in Shelly will be executing processes, rather than the extra overhead of a nested Reader.

yep, I have never heard a complaint about speed or CPU usage, just memory.

gregwebs commented 10 years ago

Looks great, thank you! I will wait for word on how porting your code goes.

I think we should create a flag for the test suite that will change the import with CPP so that both can be tested.

jwiegley commented 10 years ago

Ok, I will both port my code and conditionalize the tests today, and then we'll see how it goes. I think the Lifted layer is so "zero logic" that unless I've gotten some of the instances wrong, it should be pretty solid.

gregwebs commented 10 years ago

yeah, that is why I don't think we need to automatically run it every time by default, but just have it as a flag.

jwiegley commented 10 years ago

@gregwebs I've ported my application to using this type:

type App a = ReaderT Options Sh a

It worked like a charm! I didn't have to change any code except to remove all the Options arguments in my functions. This did reveal one minor bug, which I've fixed above.

gregwebs commented 10 years ago

When compiling this, I get:

src/Shelly/Base.hs:77:10:
    Not in scope: type constructor or class `Catch.MonadThrow'

Is there a version bound missing?

jwiegley commented 10 years ago

Very possibly. I'll try to get that fixed tonight. In fact, I need to add lower and upper version bounds for all the new dependencies.