Fix #453: log files when scene is not specified #521
Conversation
charlie4fun
force-pushed
the
fix_453
branch
4 times, most recently
from
977dfad to
09864e6
Compare
|
I don't know how to rerun particular tests in CI (https://github.com/ManimCommunity/manim/actions/runs/292167071). As they show GitHub Actions has encountered an internal error when running your job. When i was rerunning them altogether they show same error as mentioned above for a different set of tests |
| class SquareToCircle(Scene): | ||
| def construct(self): | ||
| self.play(Transform(Square(), Circle())) | ||
|
|
||
|
|
There was a problem hiding this comment.
@huguesdevimeux could you make sure this change makes sense? I've sort of lost track of tests
There was a problem hiding this comment.
It makes sense. Renaming the file will provide more clarity.
manim/config/config.py
Outdated
| scene_file_name = os.path.basename(args.file).split(".")[ | ||
| 0 | ||
| ] # takes filename and removes extension |
There was a problem hiding this comment.
Suggestion: I think this comment is unnecessary. Also, removing it will make black format this line a lot more prettily.
manim/config/config.py
Outdated
| @@ -157,9 +157,18 @@ def _parse_config(config_parser, args): | |||
| set_rich_logger(config_parser["logger"], file_writer_config["verbosity"]) | |||
| if file_writer_config["log_to_file"]: | |||
| # IMPORTANT note about file name : The log file name will be the scene_name get from the args (contained in file_writer_config). So it can differ from the real name of the scene. | |||
There was a problem hiding this comment.
It sounds like your PR should change what this comment says.
| 0 | ||
| ] # takes filename and removes extension | ||
| log_file_name = ( | ||
| f"{scene_file_name}_{scene_name_suffix}.log" |
There was a problem hiding this comment.
Praise : Naming the log depending on the suffix is a very good idea!
|
@leotrs I made some changes addressing your comments on PR, should something more be done in order to merge it? |
List of Changes
manimwithout scene name fixedtests/test_logging/basic_scenes.pysplit into two files, to enable triggering ofSquareToCirclewithout scene name inmanimcommandmanimwithout scene name addedI guess as
--log_to_filedoes not exist in original library and was not released yet, fix related to it should not be reflected indocs/source/changelog.rstMotivation
It is aimed to fix issue #453
Explanation for Changes
Now log file name would consist of two parts <name_of_animation_file>_<name_of_scene>.log
<name_of_scene> - would be optional, only if scene name was provided on
manimcallTesting Status
all tests passed, Ubuntu 18.04
Further Comments
seems like
WriteStuffscene fromtests/test_logging/basic_scenes_wirte_stuff.pywas never triggered, nevertheless I did not remove itAcknowledgement