Skip to content

Conversation

@miloth
Copy link
Contributor

@miloth miloth commented Apr 26, 2024

Hi there, I was looking at this repo and the sister on concerning matplotlib. I saw that there is this outstanding issue: catppuccin/matplotlib#2.

This PR aims at solving that. Looking at the repo structures, I believe they could live as one, rather than two separates. I checked it with ruff, mypy and pytest. It should be alright.

Main changes:

  • Ported over all matplotlib functionalities.
  • Modified the code to work with the built-in PALETTE object.
  • Conditional import loading the styles only if matplotlib is in the modspace (using importlib).
  • Added test to check the styles can be added to the matplotlib library.
  • Reworked the examples/assets gen to use built-in numpy functions.
  • Added a subprocess call to ruff in the build script for formatting the autogenerated file with the palettes.
  • Added the py.typed marker to the repo to identify it as typed for mypy usage in packages that use this as a dependency.
  • Added the poetry config to have the virtual environment generated locally in the .venv folder in the root of the project.

Notes:

  • How do you and @brambozz want to properly credit/license the files?
  • Do you think other tests should be added?
  • Each commit should have a single limited scope, to make it easier to review the PR.
  • I didn't look into the docs yet. Where and how should the plotting stuff be added there? Same goes for the README.
  • Similar thing with the CIs. I didn't look at them, feel free to let me know if there's anything that needs attention.
  • In order to add an matplotlib in an "easy" way to the toml, I removed 3.8 from the supporte Python versions, since it'll be EOL in just a few months. It could be made to work, but I don't think it is worth for the little time 3.8 is supported.

@brambozz
Copy link
Collaborator

brambozz commented May 8, 2024

Hi!

Great stuff! Good idea to port the matplotlib functionality here, many python users probably use it at some point anyway. I will try to have a look at the PR somewhere next week, but I suppose since I don't maintain this package it's mostly up to @backwardspy :)

I would like to exchange thoughts about findability. Having a separate repo for matplotlib is nice, because in the first instance I don't think people expect a python catppuccin port to include matplotlib. But code-wise it fits well in this repo. Maybe it would work best to keep the matplotlib repo alive, perhaps archived or with only a README? I.e. link somehow from catppuccin/matplotlib to catppuccin/python, because I feel like quite some people will land on matplotlib but not python.

As to (some) of your notes:

How do you and @brambozz want to properly credit/license the files?

For me it's fine to put my name into e.g. the Thanks to section, mentioning the matplotlib part.

I didn't look into the docs yet. Where and how should the plotting stuff be added there? Same goes for the README.

As far as I see, there is only a README for this repo, same for mine. An option could be to simply put the contents of my README to this one, as a separate matplotlib section. I think I would prefer that to putting it behind an extra click (e.g. in a wiki or so), because now people can find catppuccin/matplotlib and directly have a bit of example code and are ready to go. This was my intention, to directly show it is easy to use, so people will actually use it.

What I mentioned before could also be an option: to make catppuccin/matplotlib only a README, with install instructions for catppuccin/python and showing only matplotlib functionality usage.

@miloth
Copy link
Contributor Author

miloth commented May 8, 2024

Hi there,

  • Talking about how easy it is to find, it would be great if:
    • The previous package features a update as soon as this would be released. Including specific migration guides on how to move to the main package, as well as adding deprecation warnings when used.
    • Add proper tags in the repository to show it also features a plotting tool.
    • Add the description of the plotting to README.md. Definitely needs some porting over from the original package.
    • Note that the package could have extras, so you could call pip install catppuccin -extras matplotlib. The functionalities would still be there if you have matplotlib already, but specifying the extra makes it clear and makes sure you fetch the plotting packges if not already in your environment.
  • Docs. I think most of the project is properly documented in the docstrings and typed. I can do a small cleanup in a separate PR and fire up a CI on GH Actions to generate the Pages, with either pydoctor or mkdocs. I'd prefer the first as it better shows the attributes of the objects. The only downside is that I would need to move the README from md to rst. Not a big deal for me, but I on't know how people feel on rsts.
  • Credit. Easily done, I'll add something for you guys to review.

EDIT: It appears there is a Pages workflow there, but it doesn't generate a website yet, only the CSS for the 4 flavors. Yet, it shouldn't be that difficult to add the docs generation to that one. Happy to give it a go after this PR.

Copy link
Collaborator

@brambozz brambozz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the late reply, busy times...

Looks great! Very neatly done and makes a lot of sense to include the matplotlib functionality likes this.

I suppose this is anyway not really for me to approve/merge, so I will leave that to others.

Once this gets merged, I will update the catppuccin/matplotlib package appropriately with deprecation, and a link to this repo in its README.

Documentation also sounds good, I can help out there if you want. The original README I think contains enough 'quick' examples that get people started. It would be nice to have something like that, so that people can use it rightaway.

@backwardspy
Copy link
Member

your review & approval is very much appreciated @brambozz, thank you!

Copy link
Member

@backwardspy backwardspy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your hard work on this PR! i think it looks good, just one issue to clear up and then we should be good to merge this in.

@miloth miloth requested a review from backwardspy June 17, 2024 21:11
@miloth
Copy link
Contributor Author

miloth commented Jun 17, 2024

The PR catppuccin/matplotlib#3 on the other repo introduces a deprecation warning and redirects the users to this repo.

@backwardspy backwardspy merged commit 580dabc into catppuccin:main Jun 18, 2024
@backwardspy
Copy link
Member

backwardspy commented Jun 18, 2024

huge thanks to the both of you @brambozz and @miloth for your work on the matplotlib port and on bringing it into this repo.

i've credited you both in the readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants