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

formatting: use emoji 2.0 style #119

Merged
merged 1 commit into from
Dec 12, 2022
Merged

formatting: use emoji 2.0 style #119

merged 1 commit into from
Dec 12, 2022

Conversation

ofalk
Copy link
Contributor

@ofalk ofalk commented Dec 7, 2022

use language='alias' instead of use_aliases=True, which was deprecated in 1.7.0,
see also: https://github.com/carpedm20/emoji/blob/v1.7.0/emoji/core.py

Pretty straight forward change. Let me know if there is anything else that I can/should do, since I already have the fork open.

@dgw dgw added this to the 0.4.8 milestone Dec 7, 2022
@dgw dgw added the bugfix label Dec 7, 2022
@dgw
Copy link
Member

dgw commented Dec 7, 2022

Indeed, it is straightforward to do it this way. I suggest updating the README to document that emoji 2.0 or higher is now required for this to work, and update the commit message so the actual change is shown in the top line.

Commit message example:

formatting: use `emoji` 2.0 style

The `use_aliases` argument was deprecated in `emoji` 1.7 and removed
in 2.0. It was replaced with the option `language='alias'`.

Fixes #118. See also:
https://github.com/carpedm20/emoji/blob/d8bbfe455c6fcd12b96ed1dce6e0978fe7a47431/emoji/core.py#L95-L98

(We do obey the 72-character wrapping rule, except for URLs, because we want them to stay clickable.)

@dgw dgw changed the title Fix #118 formatting: use emoji 2.0 style Dec 7, 2022
@ofalk
Copy link
Contributor Author

ofalk commented Dec 7, 2022

Hey!
Makes sense! But I cannot commit to do this before Friday, if that's fine?

@dgw
Copy link
Member

dgw commented Dec 7, 2022

No big rush! As I said before, the workaround is easy (downgrade to emoji<2).

@ofalk
Copy link
Contributor Author

ofalk commented Dec 9, 2022

There we go. Updated the commit message.

@dgw
Copy link
Member

dgw commented Dec 9, 2022

Readme update?

@ofalk
Copy link
Contributor Author

ofalk commented Dec 9, 2022

Crap. I knew there was something I missed. 😂
I'll update the readme in the next days!

@ofalk
Copy link
Contributor Author

ofalk commented Dec 10, 2022

Alrighty. What about this?

@dgw
Copy link
Member

dgw commented Dec 10, 2022

Rather than add a new section, I was expecting an update to the existing mention here:

sopel-github/README.md

Lines 47 to 49 in 588db54

If you have [the `emoji` package](https://pypi.org/project/emoji/) installed,
most `:emoji_name:`s will be converted to Unicode emoji in the output. (GitHub
supports some non-standard names that this plugin doesn't handle yet.)

And if you want to give an install command with >= specified, it has to be quoted: pip install 'emoji>=2.0' (otherwise pip will install just emoji, and Bash will send its output to a file called =2.0).

Realistically though, I wouldn't worry too much about getting the readme perfect. That feature really should be a setuptools extra, with the requirement specified in package metadata. This is largely why I had in mind a minimal update to the existing mention of using the emoji package, because it's not going to stay this way for very long.

@ofalk
Copy link
Contributor Author

ofalk commented Dec 10, 2022

OK, we're getting there! Update README again.

@dgw
Copy link
Member

dgw commented Dec 10, 2022

Hmm, bad force-push?
image

The `use_aliases` argument was deprecated in `emoji` 1.7 and removed
in 2.0. It was replaced with the option `language='alias'`.
Update README.md to reflect this requirement.

Fixes sopel-irc#118. See also:
https://github.com/carpedm20/emoji/blob/d8bbfe455c6fcd12b96ed1dce6e0978fe7a47431/emoji/core.py#L95-L98
@ofalk
Copy link
Contributor Author

ofalk commented Dec 10, 2022

Seems, I missed to amed the commit :-(

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I gotta think about a release plan for this, the setuptools extra, and the other bug fixes I'm working on—but your part is all done. Thanks!

@ofalk
Copy link
Contributor Author

ofalk commented Dec 10, 2022

Thanks a lot @dgw ! Esp. for your patience to get this done.

@dgw dgw merged commit 3a987e3 into sopel-irc:master Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants