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

Remove python2 compat codes, introduce dataclasses and type hints for bin/update-tables.py #58

Closed
wants to merge 15 commits into from
Closed

Remove python2 compat codes, introduce dataclasses and type hints for bin/update-tables.py #58

wants to merge 15 commits into from

Conversation

GalaxySnail
Copy link
Collaborator

No description provided.

@lgtm-com
Copy link

lgtm-com bot commented Mar 6, 2022

This pull request introduces 1 alert when merging 53b0dd1 into 4d45480 - view on LGTM.com

new alerts:

  • 1 for Unused import

@jquast
Copy link
Owner

jquast commented Mar 8, 2022

Thanks for this work. Can I please turn your attention to the use-jinja branch? It is mostly completed and simplifies this script a great deal by using jinja2 templates. https://github.com/jquast/wcwidth/compare/use-jinja

The jinja2 branch is to start generating C code and improve performance of this library. If you are interested in helping with that effort, I have been looking for a co-maintainer, your work is good, are you interested?

@GalaxySnail
Copy link
Collaborator Author

Thanks for this work. Can I please turn your attention to the use-jinja branch? It is mostly completed and simplifies this script a great deal by using jinja2 templates. https://github.com/jquast/wcwidth/compare/use-jinja

Yes. Templates can be more readable than code generator. I haven't used jinja2 before, so let me read its documents first.

The jinja2 branch is to start generating C code and improve performance of this library.

I agree, C code will be good. Which should we use, Python/C API or Cython? Python/C API has some disadvantages (for example, reference counting), and there is a WIP replacement named hpy, which may be better.

If you are interested in helping with that effort, I have been looking for a co-maintainer, your work is good, are you interested?

It's my pleasure. 😉

@jquast
Copy link
Owner

jquast commented Mar 16, 2022

Thanks for your help, access granted

I hope to polish up use-jinja branch and merge this weekend which will include the newest unicode version. I've used both Python/C API and Cython lightly in the past, I've been happier with Cython. I would want whichever is more portable or likely to succeed in build on alternate platforms.

Please forgive me if I take the transition to C a bit slow, because this library is downloaded a lot I want to be sure the installation process very forgiving, to fallback to pure python if no compiler is present, to try to provide binary wheels for top platforms & versions and to do platform testing

@jnoortheen
Copy link

@jquast you may be interested in mypyc . It was recently used in black as well.

@GalaxySnail
Copy link
Collaborator Author

you may be interested in mypyc . It was recently used in black as well.

I think the algorithm will be implimented in C, and either Python/C API or cython is only for wrapping the C code in a Python extension module.

AFAIK, mypyc doesn't support pypy. At least, it haven't been tested on pypy, and mypy doesn't distribute any mypyc-compiled wheels for pypy on pypi.org either (There's only cp ABI for cpython). And I doubt that mypyc can provide speed up as much as C code does.

Cython does support pypy (via pypy's cpyext). And cython has a pure python mode (but I haven't used it before, I'm not sure if we can make both cython and mypy's type checker happy). Futher more, if one day cython supports HPy backend, we can get more performance improvement on pypy (and graalpython, and some others).

GalaxySnail added a commit that referenced this pull request Mar 21, 2022
GalaxySnail added a commit that referenced this pull request Mar 21, 2022
GalaxySnail added a commit that referenced this pull request Mar 21, 2022
Cherry picked from commit 50e3d41 and
commit e7de09f (GH-58)
GalaxySnail added a commit that referenced this pull request Mar 21, 2022
Cherry picked from commit 0a9cdf7 and
commit 82f550f (GH-58)
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