Deprecate stack-setup-yaml

[decentral1se]

Closes #2647. Hope that covers it.

[decentral1se]

Oh wait, it doesn't compile .... :face-palm:. OK, let's try that.

Please see these messages for a query related to me not killing your Travis runner time.

[Blaisorblade]

Deprecating --stack-setup-yaml should mean still supporting it (with some sort of warning in the help text and at runtime), but the code doesn't seem to do that... In particular I doubt you want to have a separate field—instead of SetupCmdOpts <$> I'd use a smart constructor for SetupCmdOpts.

[decentral1se]

Thanks for tips @Blaisorblade but I am a little confused.

Regarding what the stack setup --help should look like, would it be:

λ  ~  stack setup --help
Usage: stack setup [GHC_VERSION] [--[no-]reinstall] [--[no-]upgrade-cabal]
                   [--stack-setup-yaml ARG] [--ghc-bindist URL] [--help]
  Get the appropriate GHC for your project
Available options:
  --stack-setup-yaml ARG   <deprecation notice>
  --setup-info-yaml URL   <new explanation>
...

Not sure how I can achieve this with a smart constructor given I need a new field to represent both? Or should the --help just show --setup-info-yaml and the code will still accept --stack-setup-yaml and output a runtime warning.

[Blaisorblade]

I'd have said expected URL, not ARG in --stack-setup-yaml URL. But otherwise yes, that's the message that makes sense to me.

Disclaimer: I haven't tried this out, but at least here's what I imagined—with a bit more detail added. Feel free to ignore tips that don't actually make sense (I'm also learning optparse-applicative 😉 ).

Or should the --help just show --setup-info-yaml and the code will still accept --stack-setup-yaml and output a runtime warning.

No, deprecations are usually announced, just like in API—how do you study existing scripts otherwise? I also wouldn't know how to achieve that.

Not sure how I can achieve this with a smart constructor given I need a new field to represent both?

Well, why a new field? Again, that sounds bad. What I had in mind was (ignoring other fields):

setupCmdOpts :: Maybe String -> Maybe String -> SetupCmdOpts
setupCmdOpts stackSetupYaml setupInfoYaml = SetupCmdOpts (stackSetupYaml <|> setupInfoYaml <|> Just defaultStackSetupYaml).

setupCmdOpts <$> ... <*> ...

I wondered how to build parsers for Option String though, but that seems doable through optional—it has the right type and it seems to have the right behavior.

And to get the right output, it's probably best to leave the default in setupInfoYaml:

setupCmdOpts :: Maybe String -> String -> SetupCmdOpts
setupCmdOpts stackSetupYaml setupInfoYaml = SetupCmdOpts (stackSetupYaml <|> setupInfoYaml).

setupParser = setupCmdOpts
    <$> OA.optional (OA.strOption
            ( OA.long "stack-setup-yaml"
           <> OA.metavar "URL"
           <> OA.help "DEPRECATED in favour of 'setup-info-yaml'"))
    <*> OA.strOption
            ( OA.long "setup-info-yaml"
           <> OA.metavar "URL"
           <> OA.help "Alternate URL or absolute file path for various tools stack relies on (GHC, msys2)"
           <> OA.value defaultStackSetupYaml
           <> OA.showDefault )

[Blaisorblade]

Forgot: the above doesn't allow per se a runtime warning. If needed we might need to store a boolean flag in SetupCmdOpts—but that could be computed as isJust stackSetupYaml. EDIT: I've asked on #2647 (comment).

[Blaisorblade]

BTW, what I proposed is more complicated than having separate fields for the options. The problem with having two fields is making sure they're not treated differently by the command—so at some point stackSetupYaml <|> setupInfoYaml must happen, and preferably early enough that most of the code can't possibly forget it—that's a facet of designing types to prevent bugs.

Generally speaking, the option parser is IMHO still the best place, but having two fields in a new type SetupCmdOpts' from which one produces a SetupCmdOpts would not IMHO be absurd. EDIT: or really anything that achieves this guarantee.

Finally: stackSetupYaml <|> setupInfoYaml gives priority to stackSetupYaml, not sure we want that O_O. Boy is this tricky.

[decentral1se]

Thanks for this brain dump!!! I will work towards something useful :)

[decentral1se]

@Blaisorblade Is skipping the run time warning acceptable? Given that OA.value gurantees a value is present, it's hard to detect when the user passed --stack-setup-yaml?

[Blaisorblade]

Not 100% sure on that, that's why I asked in #2647. I just realized that you answered me...
I'd wait a bit in case others want to answer, but I guess I'm fine with it, since we're not in a hurry to drop the deprecated options. (I'm not sure there's a pattern).

[decentral1se]

I just realized that you answered me...

😀

[mgsloan]

LGTM, thanks!