gregwebs / Shelly.hs

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

Fix ShellCmd String arguments #221

Closed cunningdefenestrator closed 1 year ago

cunningdefenestrator commented 1 year ago

Fixes #143.

Changelog:

cunningdefenestrator commented 1 year ago

Looks likes this succeeds for everything but ghc 8.0.2--I'm not sure why because AFAICT the instance selection rules haven't changed. How should I proceed?

andreasabel commented 1 year ago

@cunningdefenestrator : Thanks for the PR. I couldn't push to your branch (permission denied). Could you give me permission? In the meanwhile, I backed this PR up to:

andreasabel commented 1 year ago

Looks likes this succeeds for everything but ghc 8.0.2--I'm not sure why because AFAICT the instance selection rules haven't changed. How should I proceed?

GHC 8.0 can be considered a legacy GHC by now, so a pragmatic solution would be to drop GHC 8.0. (One could also investigate whether/how the broken files could be fixed for GHC 8.0.)

You suggest a major-major version bump to 2.0.0---do you expect a lot of breakage?

cunningdefenestrator commented 1 year ago

@andreasabel I've sent an invite.

If we're being strict about SemVer this deserves a major version bump since I changed the signature of CmdArgs. In practice, I don't think too many people would use CmdArgs directly, but I'll defer to your judgment here.

I suspect that re-enabling IncoherentInstances as a top-level pragma might work for GHC 8.0, but given that it's deprecated I think dropping support would be better.

andreasabel commented 1 year ago

@andreasabel I've sent an invite.

Thanks! For now, it is more convenient for me to continue on #223, but I might come back to this. (UPDATE: can work here after I force-pushed.)

If we're being strict about SemVer this deserves a major version bump since I changed the signature of CmdArgs. In practice, I don't think too many people would use CmdArgs directly, but I'll defer to your judgment here.

Yes, definitely a major version bump. I it only that shelly (like most Haskell packages) follow the Haskell PVP whether the first two numbers are major, the third is minor, and the fourth is patch version. Thus, 1.12.0 would already be a major version bump.
If lots of breakage is expected, developers might opt for even a major-major version bump, like 2.0.0.

I suspect that re-enabling IncoherentInstances as a top-level pragma might work for GHC 8.0, but given that it's deprecated I think dropping support would be better.

I think if we drop 8.0 then a simple major bump to e.g. 1.12.0 would be sufficient for sure, but it might even be sufficient if we keep 8.0 and alert of the breakage, suggesting IncoherentInstances as a workaround.

andreasabel commented 1 year ago

@cunningdefenestrator : Shouldn't Shelly.Pipe then be changed in the same way? https://github.com/gregwebs/Shelly.hs/blob/d0b16a601b7f829431938b1ebaa699419a193653/src/Shelly/Pipe.hs#L612-L615 Also, since this is a breaking change, could you draft a migration guide, e.g. on top of the ChangeLog.md file?

cunningdefenestrator commented 1 year ago

After familiarizing myself with the Haskell PVP, this doesn't justify a major.major bump, so I dropped the version back down to 1.12.0. I've also added a note about migrating existing instances to the changelog, and I dropped support for GHC 8.0.

I'll make the corresponding change to Shelly.Pipe next.

cunningdefenestrator commented 1 year ago

@andreasabel Apologies for the long turnaround, but I've made the Shelly.Pipe changes.

andreasabel commented 1 year ago

Candidate: https://hackage.haskell.org/package/shelly-1.12.0/candidate

Maybe toTextArgs should get a @since 1.12.0 annotation: https://hackage.haskell.org/package/shelly-1.12.0/candidate/docs/Shelly.html#v:toTextArgs

andreasabel commented 1 year ago

I reset this branch to remove my last 3 commits pertaining to the release of 1.12.0.