-
Notifications
You must be signed in to change notification settings - Fork 81
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
base: master
Are you sure you want to change the base?
Conversation
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) |
@JotaRandom Should I update it? Which of the fixes should appear there? |
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. |
@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 |
I switched to using a raw docstring as @maclermo suggested and updated the changelog. Is there anything else left to do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
There was a problem hiding this 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 !
merge? |
still seeing the invalid sequence warning.
the warning appears for every ponysay option |
@SamuelCano03 Are you getting the exact same warning as in #314? Does it also appear when you run src/balloon.py directly? |
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? |
@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). |
@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? |
@SamuelCano03 Yup! You would need to uninstall the Arch package, download the source code from the fork and run |
sudo merge |
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.