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

Pass force_draw into drawable() #371

Merged
merged 1 commit into from Feb 10, 2022
Merged

Pass force_draw into drawable() #371

merged 1 commit into from Feb 10, 2022

Conversation

djc
Copy link
Collaborator

@djc djc commented Feb 10, 2022

This is a regression from my work on drawing into draw states. Specifically, before 57e6ee2 force_draw was passed in as part of the ProgressDrawState, but after that commit it was no longer handled correctly. Fixes #368.

@chris-laplante I think this is pretty much what you were suggesting, right? If you want to give it a quick look that would be great.

@chris-laplante
Copy link
Collaborator

This is a regression from my work on drawing into draw states. Specifically, before 57e6ee2 force_draw was passed in as part of the ProgressDrawState, but after that commit it was no longer handled correctly. Fixes #368.

@chris-laplante I think this is pretty much what you were suggesting, right? If you want to give it a quick look that would be great.

Exactly what I was thinking! Thanks for taking care of it :)

@djc
Copy link
Collaborator Author

djc commented Feb 10, 2022

Thanks for the quick review!

@djc djc merged commit beb5836 into main Feb 10, 2022
@chris-laplante
Copy link
Collaborator

chris-laplante commented Feb 10, 2022

@djc: Question for you. I'm going to develop a (eventually comprehensive) test suite for indicatif based on the in-memory work I did. They will be a fair bit of code (though I plan on writing a macro or two to reduce boilerplate), so I'm thinking they should go in the top-level tests/ directory.

The two existing tests in in_memory.rs would then also move to tests/.

What do you think?

@djc
Copy link
Collaborator Author

djc commented Feb 11, 2022

For any tests that don't need access to private APIs, putting them in an integration test (that is, in the top-level tests directory) makes sense to me! Thanks for your efforts on improving test coverage.

Out of curiosity, what's your motivation/use case for indicatif?

@chris-laplante
Copy link
Collaborator

Out of curiosity, what's your motivation/use case for indicatif?

I built a tool internally in Rust and when I first looked at indicatif (about a year ago) it didn't really meet my needs, so I decided to write my own progress library. I never open-sourced it since it was always a bit hacky. Over time indicatif has evolved and I saw it would become easier to just add the functionality I needed to indicatif rather than continuing to maintain my own library :).

@djc
Copy link
Collaborator Author

djc commented Feb 16, 2022

Nice!

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.

regression: finish_and_clear is broken in rc.2 and current main branch
2 participants