pretty up src/Stack/Package.hs

[kadoban]

Use $pretty* functions where appropriate in src/Stack/Package.hs.

Related to #2650

b85364d is probably the commit most in need of review, had to do some things I barely understood to get that working.

• Any changes that could be relevant to users have been recorded in the ChangeLog.md
• The documentation has been updated, if necessary.

[kadoban]

Hmm, somehow I thought there was more to do in that module. This might be less WIP than I thought. I still haven't really reviewed my work though, and there's a few TODOs I added that I can maybe resolve if I look around a bit. Will try to do that tomorrow.

[mgsloan]

LGTM! Just one comment to address. I like the Ctx refactoring.

[mgsloan]

I think display can be used instead because there is an instance for Path t File.

[kadoban]

Nice catch, thanks!

[kadoban]

Did that one change, nice catch.

Cool, thanks. Was hoping that Ctx part made sense. I might actually be getting a handle on what those Has* typeclasses mean, heh. They seem quite nice in practice. Need to learn more lens sometime still though.

[kadoban]

This is unfortunate, I can't figure out exactly why this can't be display instead of dispOne in this function. I get the error:

    • Couldn't match type ‘Ann (Path b t)’ with ‘AnsiAnn’
      Expected type: Path b t -> AnsiDoc
        Actual type: Path b t -> Doc (Ann (Path b t))
    • In the first argument of ‘map’, namely ‘display’
      In the first argument of ‘bulletedList’, namely
        ‘(map display rest)’
      In the second argument of ‘(<>)’, namely
        ‘bulletedList (map display rest)’
    • Relevant bindings include
        rest :: [Path b t] (bound at src/Stack/Package.hs:1146:29)
        candidate :: Path b t (bound at src/Stack/Package.hs:1146:19)
        warnMultiple :: DotCabalDescriptor
                        -> Path b t -> [Path b t] -> m ()
          (bound at src/Stack/Package.hs:1146:1)

when I try.

[kadoban]

Must be something about Path b t being polymorphic? A little confused. Adding a Display (Path b t) constraint to the function type doesn't help either.

[mgsloan]

Yeah, it's because it's polymorphic. I think adding Display (Path b t) to the function's constraints should solve the problem. Alternatively, just make it monomorpic, I think it's just used for (Path Abs File) or something.

[kadoban]

Hmm, thanks for looking at it. I thought I tried that constraint and got the same error. Very possible I screwed it up though. I'll investigate more.

[mgsloan]

LGTM. Nice idea to use hlint for this! Though, I think there could be cases where fromString . toFilePath is used to create a type other than AnsiDoc. Not a big deal though, lets see if those cases ever crop up.

Will need to be updated due to changes merged in #3389

[mgsloan]

Yeah, it's because it's polymorphic. I think adding Display (Path b t) to the function's constraints should solve the problem. Alternatively, just make it monomorpic, I think it's just used for (Path Abs File) or something.

[kadoban]

LGTM. Nice idea to use hlint for this! Though, I think there could be cases where fromString . toFilePath is used to create a type other than AnsiDoc. Not a big deal though, lets see if those cases ever crop up.

Thanks! Yeah, hopefully not many. If there's only a few we can just add exceptions for them. Can definitely re-evaluate if they pop up all over.

Will need to be updated due to changes merged in #3389

Hopefully just did that, crossing fingers for the CI builds.

Edit: hmm, not going to build. Will have a bit more work to do unfortunately. Need to either make the pretty* function types more general, or make a lot of the functions in s/S/Package.hs more specific. Or something.

[mgsloan]

Generalizing to something like (HasCallStack, HasRunner env, MonadReader env m, MonadLogger m) should do the trick

[kadoban]

Yep, that seemed to do the trick, thanks. Now to investigate that Display (Path b t) thing while waiting for CI.

[mgsloan]

Cool, looks like CI is passing, feel free to merge this.

[kadoban]

That works, I'll just come back to the Display thing for Path b t later, there's a TODO in there at least. can't seem to figure that one out though, heh.

On to finishing up terminal detection I hope :)