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

--save_last_frame appears twice in options #3174

Closed
dkaszews opened this issue Mar 2, 2023 · 2 comments · Fixed by #3528
Closed

--save_last_frame appears twice in options #3174

dkaszews opened this issue Mar 2, 2023 · 2 comments · Fixed by #3528
Labels
good first issue Good for newcomers maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements

Comments

@dkaszews
Copy link

dkaszews commented Mar 2, 2023

Description of bug / unexpected behavior

When using manim render --help, you can see --save_last_frame appearing twice, once marked as deprecated:

  -s, --save_last_frame
...
  -s, --save_last_frame          Save last frame as png (Deprecated).

This is also visible i n reference manual.

Expected behavior

The option appears exactly once

How to reproduce the issue

Code for reproducing the problem
> manim render --help | grep 'save_last_frame'

Additional media files

Images/GIFs

Logs

Terminal output
PASTE HERE OR PROVIDE LINK TO https://pastebin.com/ OR SIMILAR

System specifications

System Details
  • OS (with version, e.g., Windows 10 v2004 or macOS 10.15 (Catalina)):
  • RAM:
  • Python version (python/py/python3 --version):
  • Installed modules (provide output from pip list):
PASTE HERE
LaTeX details
  • LaTeX distribution (e.g. TeX Live 2020):
  • Installed LaTeX packages:
FFMPEG

Output of ffmpeg -version:

PASTE HERE

Additional comments

This is probably a code cleanup issue, as you can clearly see the option being declared twice in render_options.py:

# line 61
    option("-s", "--save_last_frame", is_flag=True, default=None),
# line 125:131
    option(
        "-s",
        "--save_last_frame",
        default=None,
        is_flag=True,
        help="Save last frame as png (Deprecated).",
    ),

I can create a PR to remove one of them, just need info which one should be removed - the first one without help, or the second one marking it deprecated (easiest to just remove the "deprecated" keyword as it's formated nicely, and get rid of the first one).

Should I also report it as a bug in cloup that it doesn't check for duplicates?

@github-project-automation github-project-automation bot moved this to 🆕 New in Dev Board Mar 31, 2023
@MrDiver
Copy link
Collaborator

MrDiver commented Mar 31, 2023

It is not really a bug that it doesn't check for duplicates. If you add the same command twice then it's not really cloups fault.

Just remove one of them and see if it still works as intended. If it does then make a PR!

@behackl behackl added good first issue Good for newcomers maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements labels Nov 12, 2023
@behackl
Copy link
Member

behackl commented Nov 12, 2023

The -s flag has been deprecated long enough, it should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements
Projects
Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

3 participants