Skip to content

Improved Error in :mod:.utils.tex_file_writing #2574

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Feb 26, 2022
Merged

Improved Error in :mod:.utils.tex_file_writing #2574

merged 13 commits into from
Feb 26, 2022

Conversation

@ad-chaos
Copy link
Collaborator

@ad-chaos ad-chaos commented Feb 25, 2022 •
edited
Loading

Overview: What does this pull request change?

This removes the infamous max() arg is an empty sequence error
Added an insight for when a package is not installed
Fixes #2360

Further Information and Comments

I am not sure if

Install it using your LaTeX package manager

is a helpful enough insight. Suggestions Welcome!

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

Darylgolden
Darylgolden reviewed Feb 25, 2022
View reviewed changes
Darylgolden
Darylgolden reviewed Feb 25, 2022
View reviewed changes
Darylgolden
Darylgolden requested changes Feb 25, 2022
View reviewed changes
Copy link
Member

@Darylgolden Darylgolden 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! Can you add tests that assert the correct error messages are raised?

@ad-chaos
Copy link
Collaborator Author

ad-chaos commented Feb 25, 2022

Sure!

@ad-chaos ad-chaos changed the title Improved Error in mod:tex_file_writing Improved Error in :mod:.utils.tex_file_writing Feb 25, 2022
@ad-chaos
Copy link
Collaborator Author

ad-chaos commented Feb 25, 2022 •
edited
Loading

I sank in enough time, but I have no idea why the logs will not be produced even after passing in the --log_to_file parameter.

Edit: Was a nasty comma

@ad-chaos ad-chaos requested a review from Darylgolden February 26, 2022 03:35
Darylgolden
Darylgolden requested changes Feb 26, 2022
View reviewed changes
Copy link
Member

@Darylgolden Darylgolden left a comment

Choose a reason for hiding this comment

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

It doesn't seem like the insight was raised?

@ad-chaos
Copy link
Collaborator Author

ad-chaos commented Feb 26, 2022

Insights are logged as INFO not ERROR.
Here the verbosity is set to ERROR. Since you asked:

Can you add tests that assert the correct error messages are raised?

Setting it to INFO will include the insights

@Darylgolden
Copy link
Member

Darylgolden commented Feb 26, 2022

I think it'll be good to test that the insight is raised.

@ad-chaos ad-chaos requested a review from Darylgolden February 26, 2022 07:42
icedcoffeeee
icedcoffeeee approved these changes Feb 26, 2022
View reviewed changes
Copy link
Collaborator

@icedcoffeeee icedcoffeeee left a comment

Choose a reason for hiding this comment

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

Tested locally, LGTM! Thank you for your efforts

Darylgolden
Darylgolden requested changes Feb 26, 2022
View reviewed changes
ad-chaos and others added 2 commits February 26, 2022 13:46
@ad-chaos ad-chaos requested a review from Darylgolden February 26, 2022 08:44
Darylgolden
Darylgolden approved these changes Feb 26, 2022
View reviewed changes
Copy link
Member

@Darylgolden Darylgolden 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, LGTM!

@behackl behackl added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Feb 26, 2022
@behackl behackl merged commit 5b11a0e into ManimCommunity:main Feb 26, 2022
@ad-chaos ad-chaos deleted the max_arg branch February 28, 2022 05:12
@behackl
Copy link
Member

behackl commented Feb 28, 2022

This unfortunately broke rendering of TeX strings containing %. Trying to create Tex(r"\%") fails in v0.15.0.

@behackl behackl mentioned this pull request Feb 28, 2022
Closed
behackl pushed a commit that referenced this pull request Mar 5, 2022
@ad-chaos
Fix bug introduced in Tex rendering via #2574 (#2587)
bcfa9ac 
* Bugfix

* Changes to log data

* Add test

* Seperate test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Darylgolden Darylgolden Darylgolden approved these changes

+1 more reviewer

@icedcoffeeee icedcoffeeee icedcoffeeee approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr:bugfix Bug fix for use in PRs solving a specific issue:bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tex value error when handling compilation issue

4 participants

@ad-chaos @Darylgolden @behackl @icedcoffeeee