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

Fix visuals, --restrict #313

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix visuals, --restrict #313

wants to merge 6 commits into from

Conversation

Tonyl314
Copy link

I have fixed three things:

Default ANSI code
Ponysay was using the "21" ANSI code to reset boldness; however, this functionality does not seem to be widely supported (see this gist or this answer). On my terminal, for example, it serves as double-underline, leading to a very ugly help message. For this reason, I have replaced the code with a "0", as that is a universal reset.

Fix --restrict
There was an unmatched bracket in a pony's metadata which led to the "metadata" file not being created on setup. This fixes issue #266, making the --restrict option work again.

Backslash escaping
There was an instance of a docstring using unescaped backslashes, which in turn threw Python warnings.

@JotaRandom
Copy link
Collaborator

I will ask for anyother that want to comment to comment, but i see no issue if this is more compliant to ANSI than what we have

So the only thing remaining is updating the CHANGELOG and related stuff (changelog from the manuals/ponysay.texinfo aroung line 3413)

@Tonyl314
Copy link
Author

@JotaRandom Should I update it? Which of the fixes should appear there?

@maclermo
Copy link

maclermo commented Dec 2, 2023

I don't agree with the fix. You should use multiline strings with the r prefix (raw):

r"""
@param link:str The \-directional balloon line character
"""

That way the docstring stays intact.

src/balloon.py Outdated Show resolved Hide resolved
@teferi
Copy link

teferi commented Apr 24, 2024

@Tonyl314 Can I ask you to look at @maclermo suggestion and amend the CHANGELOG as @JotaRandom requested. The syntax warning is really annoying on 3.12 python

@Tonyl314
Copy link
Author

I switched to using a raw docstring as @maclermo suggested and updated the changelog. Is there anything else left to do?

Copy link

@teferi teferi left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Copy link

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

works for me, thanks !

@codegod100
Copy link

merge?

@SamuelCano03
Copy link

still seeing the invalid sequence warning.
ponysay 3.0.3
python 3.12.4

  1. should i downgrade python?
  2. I dont want to downgrade python.

the warning appears for every ponysay option

@Tonyl314
Copy link
Author

@SamuelCano03 Are you getting the exact same warning as in #314? Does it also appear when you run src/balloon.py directly?

@SamuelCano03
Copy link

SamuelCano03 commented Aug 14, 2024

@SamuelCano03 Are you getting the exact same warning as in #314? Does it also appear when you run src/balloon.py directly?

  • yes. it's the same warning:
➜  ~ ponysay hi
/usr/bin/ponysay/balloon.py:43: SyntaxWarning: invalid escape sequence '\-'
/usr/bin/ponysay/balloon.py:43: SyntaxWarning: invalid escape sequence '\-'

Althoug it says warning on /usr/bin/ponysay/balloon.py, i dont have that .py file. I only have on /usr/bin/ a binary called ponysay. I've installed ponysay with pacman and yay (I've tried with both). Is it bad that i don't have the balloon.py?
I've just tried installing ponysay-git, it didn't work.
OS: Arch Linux x86_64

@Tonyl314
Copy link
Author

@SamuelCano03 Oh, then you don't have my fork. The fix hasn't been merged yet, so it's only available here (needs to be built from source).

@SamuelCano03
Copy link

@Tonyl314 sorry for silly question. Does it mean that at the moment the solution is installing from source from your fork? later will be available on pacman/apt repositories?

@Tonyl314
Copy link
Author

@SamuelCano03 Yup! You would need to uninstall the Arch package, download the source code from the fork and run setup.py.
Hopefully someone can merge this soon so that the fix is easier to access…

@codegod100
Copy link

sudo merge

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.

--restrict broken in 3.0.3
7 participants