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

Refactor based on the Python implementation #3

Merged
merged 6 commits into from Mar 3, 2018

Conversation

Projects
None yet
2 participants
@johnmastro
Copy link
Collaborator

johnmastro commented Mar 2, 2018

This makes a few implementation changes plus a couple user-visible additions (adding unidecode-region and unidecode-sanitize-region).

The churn is mostly in 3c0521f, where I changed things to use data broken up and stored in the same way as Python's Unidecode module. I also updated the README appropriately and added the scripts (one in Python and one in Emacs Lisp) that I used to copy the Unidecode data (following your method of Python -> JSON -> Lisp).

If you merge these changes, the MELPA recipe will need to be updated to copy the files in the new data directory. I have a commit ready against the MELPA repository to update the recipe, which I'll submit as a pull request if you merge these changes.

These changes supersede the other pull request I submitted recently. I realized belatedly that, in Emacs, operating on a buffer the way to go (rather than lists and strings as in the Python implementation). With these changes we do that by implementing unidecode-unidecode and unidecode-sanitize in terms of the -region variants.

Please let me know if I can provide any additional information or if you'd like to see any changes.

johnmastro added some commits Feb 17, 2018

Use insert-file-contents for unidecode-chars.el
This avoids keeping the unidecode-chars.el buffer around indefinitely.
Add unidecode-region and unidecode-sanitize-region
Re-implement unidecode-unidecode and unidecode-sanitize in terms of
them.
Rename unidecode-unidecode to unidecode
Add unidecode-unidecode as an alias for backwards compatibility.

@sindikat sindikat merged commit 97816e6 into sindikat:master Mar 3, 2018

@sindikat

This comment has been minimized.

Copy link
Owner

sindikat commented Mar 3, 2018

You seem to know what you're doing. I haven't touched this code for almost 4 years, so feel free to do what you want with this code. My only question: what should be the license for this code? Python's unidecode package is GPLv2 (https://pypi.python.org/pypi/Unidecode/), I haven't used their code, but I have used their character table.

@johnmastro johnmastro referenced this pull request Mar 3, 2018

Merged

Update unidecode recipe #5351

6 of 6 tasks complete
@johnmastro

This comment has been minimized.

Copy link
Collaborator

johnmastro commented Mar 3, 2018

Thanks! I submitted a pull request to MELPA to update the recipe accordingly. I included a link to this PR; not sure if they'll want you to confirm somehow that you approve of the change to the recipe.

Regarding the license, I don't know if it's necessary, but my instinct is to go with GPLv2 just to be safe. In general I'm torn on the relative merits of copyleft licenses vs more permissive licenses, but for an Emacs package I think the possible downsides of copyleft licenses aren't very applicable. If you agree (or select another license) I can submit a PR to add the LICENSE/COPYING file and make any other updates (e.g. in the file headers).

I've set myself up to "watch" the repository in case anyone submits issues. Should I also add myself as a maintainer in the package headers?

@sindikat

This comment has been minimized.

Copy link
Owner

sindikat commented Mar 4, 2018

Yes, feel free :)

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