Literal annotation for named color maps #106
Conversation
tlambert03
left a comment
tlambert03
left a comment
There was a problem hiding this comment.
this is great! 👍
Untitled.mov
left some minor comments below
src/cmap/_colormap.py
Outdated
| # All of the things that we can pass to the constructor of Colormap | ||
| ColormapLike: TypeAlias = Union[ | ||
| str, # colormap name, w/ optional "_r" suffix | ||
| ColormapName, |
There was a problem hiding this comment.
| ColormapName, | |
| ColormapName, | |
| str, |
I know it feels like a cop-out, having added such lovely completion here. But from past experiences, I have a feeling this will result in people using # type: ignore all over the place rather than doing the research to import ColormapName to make their type checker happy. By adding str, we don't lose the lovely autocompletion and discovery.
There was a problem hiding this comment.
what would you think about making this file .pyi? We don't ever want the compiler to have to make byte-code for it. It would require/demand import inside of a TYPE_CHECKING clause. (You could also export it in cmap/__init__.py, inside of TYPE_CHECKING, for those who want to use it externally)
There was a problem hiding this comment.
(would also fix the failing 3.9 tests)
scripts/make_cmapnames.py
Outdated
| """Produce the expected script.""" | ||
| names = [] | ||
|
|
||
| for rel_path in sorted(data_dir.glob("*/record.json")): |
There was a problem hiding this comment.
since ultimately, strings passed to Colormap are compared against the catalog:
Lines 232 to 234 in fb69861
you could also use the catalog directly here, rather than parsing record.json again. This would avoid any divergence in logic for how the catalog is constructed from records.
import cmap
list(Colormap.catalog()) # all the colormap names, without the `_r`not sure if you were trying to refrain from importing cmap? I don't see any issues with that...
There was a problem hiding this comment.
I'm now using the Catalog, although I am using the normalised names for consistency with the docs. I added some logic to avoid ambiguous alias warnings.
scripts/make_cmapnames.py
Outdated
| scripts_dir = Path(__file__).resolve().parent | ||
| module_dir = scripts_dir.parent.joinpath("src", "cmap") | ||
| data_dir = module_dir.joinpath("data") | ||
| out_file = module_dir.joinpath("_colormapname.py") |
There was a problem hiding this comment.
.pyi... depending on how you felt about that suggestion in my other comment
There was a problem hiding this comment.
I've now set this up as a pyi, which passes mypy although I still get a yellow squiggly in my editor (VSCode).
|
Ah, depending on cmap means you can only run it when you have cmap installed, which isn't ideal e.g. on CI. What do you think? |
|
|
|
see #108 (comment) |
3 participants
See #105