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

specify minimal required version of rich #246

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

behackl
Copy link
Member

@behackl behackl commented Aug 4, 2020

List of Changes

  • Set minimum requirement for version of rich in requirements.txt

Motivation

Resolves #245

Explanation for Changes

See #245 (comment) and the related issue Textualize/rich#190

Acknowledgement

@leotrs
Copy link
Contributor

leotrs commented Aug 4, 2020

Great! Can you share how did you decided that 4.2.1 was the appropriate version to fix here? In general we do not want to latest version as people may not have it installed yet (or it may contain bugs), but we also do not want an old version.
cc. @naveen521kk

@Aathish04
Copy link
Member

Aathish04 commented Aug 4, 2020

Great! Can you share how did you decided that 4.2.1 was the appropriate version to fix here? In general we do not want to latest version as people may not have it installed yet (or it may contain bugs), but we also do not want an old version.
cc. @naveen521kk

Version 4.2.1 and above allow us to prevent the truncation of URLs and file paths via attributes to rich.Console().

This is required to have a uniform amount of characters in the log, so character by character comparisons can take place (for the tests).

The issue that asked for this over at rich's own repo was Textualize/rich#190 and in that, rich's author states that they have introduced the feature that we need (the one that allows us to disable truncating URLs and file paths) in v4.2.1.

@behackl
Copy link
Member Author

behackl commented Aug 4, 2020

Great! Can you share how did you decided that 4.2.1 was the appropriate version to fix here? In general we do not want to latest version as people may not have it installed yet (or it may contain bugs), but we also do not want an old version.
The issue that asked for this over at rich's own repo was willmcgugan/rich#190 and in that, rich's author states that they have introduced the feature that we need (the one that allows us to disable truncating URLs and file paths) in v4.2.1.

Precisely, this issue over there was the reason I tried with 4.2.1 -- however, I also just tried with 4.2.0 and the tests also passed there. In fact, this seems to be the first version where the additional newline in the end is not a problem: the previous version, 4.1.0, still fails.

@Aathish04
Copy link
Member

Precisely, this issue over there was the reason I tried with 4.2.1 -- however, I also just tried with 4.2.0 and the tests also passed there. In fact, this seems to be the first version where the additional newline in the end is not a problem: the previous version, 4.1.0, still fails.

Perhaps your paths weren't long enough to be truncated?

@leotrs
Copy link
Contributor

leotrs commented Aug 4, 2020

Well if they're using semantic versioning, it will probably always be better and not problematic to use 4.2.1 instead of 4.2.0. Merged!

@leotrs leotrs merged commit 7db4ef4 into ManimCommunity:master Aug 4, 2020
@behackl
Copy link
Member Author

behackl commented Aug 4, 2020

Guys! Please do the same in setup.py nobody is actually using requirements.txt. @behackl

Will provide another PR. But I do want to mention that https://github.com/ManimCommunity/manim#installing-manim-community-itself explicitly says:

Finally, run the following:

python3 -m pip install -r requirements.txt

(which is also how I installed manim, so "nobody" is not quite true. ;-))

@leotrs
Copy link
Contributor

leotrs commented Aug 4, 2020

Yeah, the requirements.txt file will be used by anyone who will try to develop on this repo.

@naveen521kk
Copy link
Member

But anyhow making in setup.py is necessary as that decides if we do pip install -e .. Also, our CI uses it.

@behackl behackl mentioned this pull request Aug 4, 2020
1 task
@behackl
Copy link
Member Author

behackl commented Aug 4, 2020

Don't worry, I was not trying to argue. :-)

Sorry for missing the setup.py in the first iteration, I was not really thinking. Follow-up: #247

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssertionError on test_logging.py solved by strip()
4 participants