-
Notifications
You must be signed in to change notification settings - Fork 241
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
introduce TermLike trait; add InMemoryTerm #354
Conversation
As I'm pretty busy, I'm going to default to not reviewing this until it's no longer a draft. If you'd like to have feedback (ideally on some specific aspect of the code/design) sooner, please ping me! |
Sounds good, I need to fix some issues with it anyway. Thanks! |
1d735de
to
a037d1e
Compare
afdff7a
to
2ce3ba8
Compare
I'd say this is ready :). |
f1a0439
to
e0cc347
Compare
e0cc347
to
8a515cf
Compare
@djc I'd say this is ready. Your recent changes did not let me avoid using |
Would you mind squashing all the fix commits into the original commits so that the commits are easier to review? |
8c0edcc
to
f1221a3
Compare
f1221a3
to
36215ed
Compare
Sure thing, done. |
@@ -29,6 +30,7 @@ tokio = { version = "1.0", features = ["time", "rt"] } | |||
[features] | |||
default = [] | |||
improved_unicode = ["unicode-segmentation", "unicode-width", "console/unicode-width"] | |||
in_memory = ["vt100"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a use case for this outside testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, so I suppose we could just put vt100 behind dev-dependencies and get rid of the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm there's an issue with that. The 1.51 version of Rust doesn't support 2021 edition, but vt100 uses that. As a feature, I was able to tweak the GitHub workflow to exclude the feature being used with 1.51. But I don't think that's possible if vt100 is just a dev-dependency. If we want to support 1.51 I think in_memory
will have to be a feature, unless there is a way I'm not seeing to limit a dev-dependency to only certain compiler versions.
(And please squash "reorder items" into "add InMemoryTerm".) |
But don't try to enable in_memory feature for 1.51 (because vt100 needs 2021 edition). Instead, manually enable improved_unicode
24e4c5d
to
f5565ed
Compare
Thanks for all the work on this! |
These changes make it possible to write tests that compare the visual output of progress bars to expected output.
The
TermLike
trait provides a common abstraction over theconsole::Term
type and the newInMemoryTerm
. The trait is made public so that users can develop their own custom draw targets, should the need arise.InMemoryTerm
is basically just a vector of lines, with a cursor that tracks the current line.Still to-do:
TermLike
traitInMemoryTerm::write_line
andInMemoryTerm::write_str
InMemoryTerm
to expose the buffer contentsInMemoryTerm
to reconstitute buffer contents asString
(will replace thebuf_contents
method intest
module)