Skip to content

Improved wording in info message issued by :meth:.SceneFileWriter.clean_cache #2027

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

Merged
merged 3 commits into from
Sep 8, 2021

Conversation

NicoWeio
Copy link
Contributor

@NicoWeio NicoWeio commented Sep 7, 2021

Overview: What does this pull request change?

This PR should fix the missing whitespace in an info message: … ago.You can change this ….

Further Information and Comments

Because this is such a tiny modification, I did not do any testing — I hope you don't mind.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@k4pran k4pran changed the title fix missing whitespace in info message Added missing space in info message for :meth:.SceneFileWriter.clean_cache Sep 8, 2021
Copy link
Collaborator

@k4pran k4pran left a comment

Choose a reason for hiding this comment

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

Thank you for this, looks good! Think we should also look at rewriting the message itself as "file(s) used by it the longest ago." doesn't read well.

@k4pran k4pran added maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! labels Sep 8, 2021
@NicoWeio NicoWeio requested a review from k4pran September 8, 2021 12:52
@k4pran k4pran changed the title Added missing space in info message for :meth:.SceneFileWriter.clean_cache Added missing space in info message for :meth:~.SceneFileWriter.clean_cache Sep 8, 2021
@k4pran k4pran merged commit e22d435 into ManimCommunity:main Sep 8, 2021
@NicoWeio NicoWeio deleted the patch-1 branch September 8, 2021 17:14
@behackl behackl changed the title Added missing space in info message for :meth:~.SceneFileWriter.clean_cache Added missing space in info message for :meth:.SceneFileWriter.clean_cache Oct 1, 2021
@behackl
Copy link
Member

behackl commented Oct 1, 2021

Looking at this change, it seems like there still is no whitespace between the two parts of the string.

@behackl behackl changed the title Added missing space in info message for :meth:.SceneFileWriter.clean_cache Improved wording in info message issued by :meth:.SceneFileWriter.clean_cache Oct 1, 2021
@NicoWeio
Copy link
Contributor Author

NicoWeio commented Oct 1, 2021

Guess the "no testing" part wasn't a great time-saver after all.
I apparently assumed the rich logger to accept the same syntax as print, i.e. the *args are concatenated using spaces.
Instead, this will probably happen:

>>> import manim
Manim Community v0.10.0
>>> manim.logger.info("Hello")
[10/01/21 13:23:28] INFO     Hello […]
>>> manim.logger.info("Hello", "World")
[…]
TypeError: not all arguments converted during string formatting

Sorry — I'll fix this in a new PR.

@behackl
Copy link
Member

behackl commented Oct 1, 2021

No worries. I just realized, we have fixed the error resulting from this in #2066, but the whitespace issue still remains I believe. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants