Skip to content

Conversation

@huguesdevimeux
Copy link
Member

@huguesdevimeux huguesdevimeux commented Aug 17, 2020

MARKED AS DRAFT BUT THE PR IS DONE.
As we don't have yet tests for videos, I prefer not to merge this right now and wait to have tests. I make this PR if anyone was annoyed by #302, so one can use this branch.

List of Changes (UP TO DATE)

Explanation for Changes

  • See above. See the comments and the code for more information.

Testing Status

  • This won't have test right now as we don't have any way to test videos. When we will (soon, I swear, I'm working on it) I'll write some tests.

Acknowledgement

@PgBiel PgBiel added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Aug 22, 2020
@ManimCommunity ManimCommunity deleted a comment from Aathish04 Aug 26, 2020
@huguesdevimeux
Copy link
Member Author

Could someone run tests locally on this PR ? In GH actions, there is a weird error message

@Aathish04
Copy link
Member

Could someone run tests locally on this PR ? In GH actions, there is a weird error message

MacOS Catalina,

E           AssertionError: The frames don't match. TextMobject has been modified.
E           Please ignore if it was intended.
E           First unmatched index is at [229 381]: [  0   0   0 255] != [ 17  17  17 255]

tests/utils/GraphicalUnitTester.py:142: AssertionError
E           AssertionError: The frames don't match. Text has been modified.
E           Please ignore if it was intended.
E           First unmatched index is at [231 398]: [ 36  36  36 255] != [ 35  35  35 255]

tests/utils/GraphicalUnitTester.py:142: AssertionError
E       AssertionError: 
E       assert False
E        +  where False = <function exists at 0x1064eff70>('/private/var/folders/4q/m6kdrdfj1xgf4kjphszzqpp40000gn/T/pytest-of-aathishs/pytest-0/test_logging_to_file0/media_temp/logs/SquareToCircle.log')
E        +    where <function exists at 0x1064eff70> = <module 'posixpath' from '/usr/local/bin/../Cellar/[email protected]/3.8.5/bin/../Frameworks/Python.framework/Versions/3.8/lib/python3.8/posixpath.py'>.exists
E        +      where <module 'posixpath' from '/usr/local/bin/../Cellar/[email protected]/3.8.5/bin/../Frameworks/Python.framework/Versions/3.8/lib/python3.8/posixpath.py'> = os.path

tests/test_logging/test_logging.py:49: AssertionError

The Logging tests fails because no log is created, for some reason. Not sure why the others fail.

@huguesdevimeux
Copy link
Member Author

Thanks so much @Aathish04

UH ? tests are not working on windows but on Linux ?!

@huguesdevimeux huguesdevimeux self-assigned this Aug 29, 2020
@huguesdevimeux huguesdevimeux added the test requested Implementation of tests are requested label Aug 29, 2020
@huguesdevimeux huguesdevimeux added this to the PyPI Release 1.0.0 milestone Aug 29, 2020
@Aathish04 Aathish04 added enhancement Additions and improvements in general refactor Refactor or redesign of existing code labels Sep 3, 2020
Comment on lines +114 to +116
@@ -11 +11 @@
from . import constants, logger, console, file_writer_config
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: after this config system is refactored, we should come back here and use tempconfig

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean ? I don't see how tempconfig could be used here. It's not, well, a temporary config (in my mind)

Comment on lines 15 to 21
def __init__(self, a, b):
self.a = 2
self.b = 3.0
self.c = [1, 2, "test", ["nested list"]]
self.d = {2: 3, "2": "salut"}

o = Obj(1, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: the init arguments a and b are not used. Was this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, my mistake. Thanks !

Co-authored-by: Leo Torres <[email protected]>
@huguesdevimeux
Copy link
Member Author

@PgBiel I adressed all your requests :D

PgBiel
PgBiel previously requested changes Sep 4, 2020
@huguesdevimeux
Copy link
Member Author

@PgBiel I removed all the semi colons, Captain. Feel free to review again.

@huguesdevimeux huguesdevimeux merged commit 5dd7bfe into ManimCommunity:master Sep 8, 2020
@huguesdevimeux huguesdevimeux deleted the fix-301 branch September 8, 2020 11:35
behackl added a commit to behackl/manim that referenced this pull request Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general pr:bugfix Bug fix for use in PRs solving a specific issue:bug refactor Refactor or redesign of existing code

Projects

None yet

4 participants