Skip to content

Added custom emoji name formatting in show function - incomplete #58179

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hannah0wang
Copy link

No description provided.

@LilithHafner
Copy link
Member

Would you explain what this PR is for and how it works in more detail, please?

@ararslan
Copy link
Member

Welcome, and thanks for your willingness to contribute! As Lilith said, it would be great to include more detail on the goals of the PR so that reviewers can review more effectively and provide more informed suggestions for the implementation.

From what I understand of the intent here, it looks like this solves the same (or at least a closely related) problem as #58181, but with slightly different approaches. A downside of the approach taken here, namely to maintain a Dict of emoji mappings, is that it requires manual upkeep to stay aligned to the current Unicode version we support. I think it would be informative to look at the implementation of #58181, which reuses existing functions that do this lookup, and it keeps the context of where this information is shown limited to the REPL. Here you're extending Base.show for Base-defined types, which is considered "piracy," and this particular case has the unfortunate consequence of changing how Chars are shown in all contexts (i.e. not just in the REPL) only if the REPL module is loaded.

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