Conversation
|
@brambozz any chance you could give this a review? you know it better than i do i think :) |
|
Yes, I will try to have a look this week! |
|
@miloth Thanks for the comprehensive PR, overall looks good to me! Especially really like the addition of a default (non-mono) colormap! That makes a lot of sense, and is something I could have used for a while. Only thing I am not a fan of is the removal of the Original: PR suggestion: @backwardspy:I think it would be good if you have a quick look too, as there are some changes to files I did not write (e.g. build.py). But looks good to me! One specific thing I spotted:
|
|
Hi @brambozz, please find the replies below:
|
|
@miloth Thanks for the update! You have me convinced, it is probably indeed cleaner and more appropriate here to keep the API minimal. Also the reasoning for the versioning is fine with me, but still something I would like to give @backwardspy the final word on. So @backwardspy: For me it's all good now! But my earlier suggestion of you having a quick look still stands :) |
backwardspy
left a comment
There was a problem hiding this comment.
thanks for your patience on this, i've got a lot going on at the moment.
got a few comments/suggestions, let me know if anything's wrong or doesn't make sense.
a couple of other general things:
- dropping support for python 3.9 and 3.10 is fine by me given 3.10 is almost EOL. would you mind updating the github actions workflows to only test against 3.11+ as well?
- i think the
DPIconstant inbuild.pyis dead now, looks like its only references were removed in this PR.
cheers!
| _palette.identifier, | ||
| [ | ||
| color.hex | ||
| for color in PALETTE.frappe.colors |
There was a problem hiding this comment.
should this be _palette.colors instead?
| for color in PALETTE.frappe.colors | |
| for color in _palette.colors |
| [ | ||
| color.hex | ||
| for color in PALETTE.frappe.colors | ||
| if color.identifier in ("blue", "teal", "yellow", "peach", "red") |
There was a problem hiding this comment.
this looks like it should reference DEFAULT_COLORMAP_COLORS
| if color.identifier in ("blue", "teal", "yellow", "peach", "red") | |
| if color.identifier in DEFAULT_COLORMAP_COLORS |
| dependencies = [] | ||
| name = "catppuccin" | ||
| version = "2.5.0" | ||
| version = "3.0.0" |
There was a problem hiding this comment.
please revert this one, release-please will handle the versioning for us :)
| version = "3.0.0" | |
| version = "2.5.0" |
| @property | ||
| def matplotlib_style(self) -> str: | ||
| """Name of the matplotlib style associated with the flavor.""" | ||
| return f"catppuccin.{self.identifier}" | ||
|
|
||
| @property | ||
| def cmap(self) -> str: | ||
| """Name of the matplotlib colormap associated with the flavor.""" | ||
| return self.identifier | ||
|
|
There was a problem hiding this comment.
i would prefer these to live inside the matplotlib extra if possible.
i think there's a case for just removing cmap; relying on identifier directly does introduce a little bit of coupling, but having this here just couples it the other way around (i.e. now the Flavor model has to know about matplotlib concepts.)
as for matplotlib_style, i'm not convinced it's that useful, but there's no harm keeping a helper for it. we could go with a style_name function inside catppuccin/extras/matplotlib.py along the lines of the following:
def style_name(flavor: Flavor) -> str:
return f"catppuccin.{flavor.identifier}"these are arguable stylistic choices, albeit ones motivated by keeping the main module clear of optional extras, but i'm open to debate on them. please let me know if you think changing them is a bad idea or you have a better way to approach it.
Matplotlib 3.11 reworked the style module, changing the APIs that were used here. This aims at solving that and a bit more:
"catppuccin.<flavor>".catppuccin.extras.matplotlibmodule. Their functionality can be replicated with a one liner of Matplotlib's APIs. All the examples now use these one liners.cmaps as well.cmaps to be colored and not mono.