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 flake8 check errors: unused imports, tab warning #68

Merged
merged 16 commits into from
Nov 28, 2022

Conversation

tdadela
Copy link
Contributor

@tdadela tdadela commented Nov 15, 2022

Description

Remove unused imports.
Use "noqa W191" to hide W191 for the REVEALJS_TEMPLATE string.
Maybe it's good idea to move this template to separate file?

I want to say congratulations – great project!

@jeertmans
Copy link
Owner

Hi @tdadela, many thanks for your contribution and your interest in Manim Slides!

Maybe it's good idea to move this template to separate file?

Do you mean a separate Python file, or something else? I thank that can be a good idea.

Maybe we should replace all tabs with spaces, and still allow for this warning to occur?

@jeertmans jeertmans added the ci Continous Integration (tests, lints, checks, ...) label Nov 16, 2022
@tdadela
Copy link
Contributor Author

tdadela commented Nov 16, 2022

I've moved the template to a separate file.

@jeertmans
Copy link
Owner

Thanks for the update @tdadela!

However, as I was afraid of, you cannot include simply include external files (such as the HTML template) and expect them to be available when installed with pip.

If you were to test manim-slides convert in a folder that is not the manim-slides repository, this would not work, see the error message:

Desktop manim-slides convert ConvertExample slides.html --open                             
Traceback (most recent call last):
  File "/home/eertmans/.local/bin/manim-slides", line 8, in <module>
    sys.exit(cli())
  File "/home/eertmans/.local/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/home/eertmans/.local/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/home/eertmans/.local/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/eertmans/.local/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/eertmans/.local/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/eertmans/.local/lib/python3.10/site-packages/manim_slides/convert.py", line 202, in convert
    converter.convert_to(dest)
  File "/home/eertmans/.local/lib/python3.10/site-packages/manim_slides/convert.py", line 124, in convert_to
    revealjs_template = self.load_template()
  File "/home/eertmans/.local/lib/python3.10/site-packages/manim_slides/convert.py", line 103, in load_template
    with open(REVEALJS_TEMPLATE_PATH, "r") as content_file:
FileNotFoundError: [Errno 2] No such file or directory: 'manim_slides/revealjs_template.html

The solution might be to set up data files, see this blog post.

Do you want to take care or this?

Again, many thanks for your time and for your enthusiasm ;D

@jeertmans jeertmans added the waiting author Waiting author response label Nov 17, 2022
@tdadela
Copy link
Contributor Author

tdadela commented Nov 17, 2022

Do you want to take care or this?

Yes
But I'm not sure if I will be doing it next weekend / next month.

@jeertmans
Copy link
Owner

No problem @tdadela! (same for #71)
There is no need to hurry for that, so ping me when you did some work, or if you later decide not to work on this anymore ;-)

@jeertmans
Copy link
Owner

Hi @tdadela, sorry to finally take on this one, but I wanted to move on next features for Manim Slides, and I prefer not to stall issues / PRs for too long ;-)

I hope you don't mind!

@jeertmans jeertmans removed the waiting author Waiting author response label Nov 28, 2022
@jeertmans jeertmans merged commit 85ea9f3 into jeertmans:main Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continous Integration (tests, lints, checks, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants