Conversation
bkirwi
left a comment
There was a problem hiding this comment.
Left a few comments, but overall on board!
Will still need to take some care with compatibility breaking, but I don't see any reason why we shouldn't be able to ship this in a compatible way...
| import com.monovore.decline.HelpFormat.Plain | ||
| import com.monovore.decline.HelpFormat.Colors | ||
|
|
||
| private[decline] trait Theme { |
There was a problem hiding this comment.
I don't follow why we need both Theme and HelpFormat - seems like users could pass in a Theme directly. (And if we can get away with fewer concepts, strong preference for that!)
There was a problem hiding this comment.
seems like users could pass in a Theme directly
I treat Theme interface as almost an implementation details - the methods in it are very tightly coupled to the help rendering logic (they're by no means the minimal set of methods required to render something), and I don't think it's beneficial (at this point) to expose this interface to users at all - because it'd increase compatibility surface.
Alternatively I could make it all 1 concept and make every method in Theme private to address the concerns from previous paragraph - but I felt like keeping Theme separate and private is the easiest w.r.t. API surface and flexibility.
There was a problem hiding this comment.
Yeah, it's true that the fact that it's not user-facing does mitigate it to a large extent. Willing to keep it as is for now...
| } | ||
|
|
||
| def optionList(opts: Opts[_]): Option[List[(Opt[_], Boolean)]] = opts match { | ||
| private[decline] case class HelpArgs( |
There was a problem hiding this comment.
Neat! This seems like something with this shape could eventually be cleaned up and made public for some of the fancier help-rendering ideas folks have had (HTML generation, etc.) but leaving it private for now seems like the right call.
|
@bkirwi happy 2025 :D I've decided to give this PR another go and make it pass through MiMa. Needless to say it's a bit of a nightmare (96c00ed#diff-87e4ae97ec5803492891d3ecd23ca3f17075130de2101c2b72bb00bf6a837accR128) but I think I got it done. |
|
While the changes are binary compatible, they're not source compatible (which I believe is allowed for a minor release in our semver conventions) – I was in fact hoisted by my own petard in my own project, where |
bkirwi
left a comment
There was a problem hiding this comment.
Yeah, wow, that does look like a hassle! Looks like it worked out, though - Mima does seem happy on my end also.
I am, nevertheless, going to request some minor changes. Sorry! Hopefully a fairly light lift, but let me know if you feel otherwise.
| /** | ||
| * Controls which sections of the Help are rendered in its string representation | ||
| */ | ||
| class RenderOptions private (opts: Flags) { |
There was a problem hiding this comment.
Conceptually, what's the difference between a format and a render-options? Is there a reason that the two aren't combined in a single type?
(One of the joys of Decline is that its API surface is pretty small, so it'll be a kindness to combine these if we can.)
There was a problem hiding this comment.
I have merged RenderOptions into HelpFormat: a00d099
Is this similar to what you had in mind?
No worries at all – and if you have any more comments withheld, let it rip, I don't want to land something that becomes a maintenance burden in the future. |
So! My original thought was that the "right" / most I still think this is a good idea, but:
So, very happy to merge this! But now you know the directional thinking too in case you end up wanting to follow up or whatever in the future. |
Yeah, this would require de-casing the This is something I'm also interested in (as I'd like to make the rendering of my CLI options prettier for the website), but I'm unlikely to tackle that at the moment either. |
|
Just wanted to confirm that this is out as |
Primary goals:
Non-goals: