Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite and simplification of Chalk core #132

Merged
merged 8 commits into from
May 16, 2024
Merged

Rewrite and simplification of Chalk core #132

merged 8 commits into from
May 16, 2024

Conversation

srush
Copy link
Collaborator

@srush srush commented May 14, 2024

I was reading about some of the more recent updates to the Python type system and thought I would try a new version of the chalk core that fixed things that bothered me. This is a rather big PR, but most of it is pretty simple.

Changes:

  • Changed the Compose node to have a list of children instead of 2.
  • Changed + to merge to the list instead of adding to the tree when possible.
  • Added a Monoid "typeclass" with a default + and empty, and a general concat. Nearly everything in chalk is a monoid now
  • Added a Maybe / MList to make these monoid'able.
  • Make Visitor define an explicit generic argument for its down / up elements. Default behavior is that the down element is a monoid and define default behavoirs for compose, empty, etc. Remove kwargs hack.
  • Replace fake placeholder TTrans types with new Self argument.
  • Remove repeated Abstract class / Interface overlaps. (this is probably incorrect, but it makes the code simpler)

Issues:

  • Currently it works with python 3.10+. Need to try with earlier versions.

Why?

  • Reason 1 - cleaner types and less code to simplify things.
  • Reason 2 - I've been playing around with this library Penzai that includes a really nice tree visualizer. with a couple of lines of code you can view the whole tree from Chalk when you draw. It's pretty slick.

https://chalk-diagrams.github.io/viz/

image

@danoneata
Copy link
Collaborator

Thank you, Sasha! I'll take a closer look at the PR in the next couple of days (on Thursday probably). The penzai visualization looks sleek! I think it can be really useful for debugging!

chalk/core.py Outdated Show resolved Hide resolved
chalk/visitor.py Outdated Show resolved Hide resolved
@danoneata
Copy link
Collaborator

  • Make Visitor define an explicit generic argument for its down / up elements.

Nice; I like that we now have the input type explicitly specified for the visit methods!

Default behavior is that the down element is a monoid and define default behavoirs for compose, empty, etc.

I was curious: for the ToSVG visitor, the return type is svgwrite.base.BaseElement; how does that the type system know that this is a Monoid?

By the way, how do you type check code? Are you still using mypy? Which version and which options?

@srush
Copy link
Collaborator Author

srush commented May 16, 2024

  • Oh the BaseElement thing might be a bug. I think MyPy doesn't have the stubs for svgwrite and so it is likely failing to check that.

  • I am using mypy 1.10, should we switch to pyright?

@srush srush merged commit 671fb74 into master May 16, 2024
2 of 4 checks passed
@danoneata
Copy link
Collaborator

Thanks for the pull request and merge!

I am using mypy 1.10, should we switch to pyright?

Hmm... I don't have a strong opinion towards one or the other. But I see that pyright reports a few errors on our codebase (although I expect these to be fixable).

@srush
Copy link
Collaborator Author

srush commented May 17, 2024

K, I think I'll fix these, bump the required version to 3.10+, update the docs, and then up the version number. Excited to check out the animation lib as well.

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.

None yet

2 participants