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

introduce TermLike trait; add InMemoryTerm #354

Merged
merged 4 commits into from Feb 4, 2022

Conversation

chris-laplante
Copy link
Collaborator

@chris-laplante chris-laplante commented Jan 20, 2022

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 the console::Term type and the new InMemoryTerm. 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:

  • Document TermLike trait
  • De-duplicate code in InMemoryTerm::write_line and InMemoryTerm::write_str
  • Add a method to InMemoryTerm to expose the buffer contents
  • Add a method to InMemoryTerm to reconstitute buffer contents as String (will replace the buf_contents method in test module)

@djc
Copy link
Collaborator

djc commented Jan 23, 2022

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!

@chris-laplante
Copy link
Collaborator Author

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!

@chris-laplante
Copy link
Collaborator Author

I'd say this is ready :).

@chris-laplante
Copy link
Collaborator Author

chris-laplante commented Feb 3, 2022

@djc I'd say this is ready. Your recent changes did not let me avoid using Box, but actually we wouldn't have wanted to avoid it anyway. My intention was always that users could implement TermLike on their own types and use them freely.

@djc
Copy link
Collaborator

djc commented Feb 3, 2022

Would you mind squashing all the fix commits into the original commits so that the commits are easier to review?

@chris-laplante
Copy link
Collaborator Author

Would you mind squashing all the fix commits into the original commits so that the commits are easier to review?

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"]
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please.

Copy link
Collaborator Author

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.

src/in_memory.rs Outdated Show resolved Hide resolved
src/in_memory.rs Outdated Show resolved Hide resolved
@djc
Copy link
Collaborator

djc commented Feb 4, 2022

(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
@djc djc merged commit bdc9401 into console-rs:main Feb 4, 2022
@djc
Copy link
Collaborator

djc commented Feb 4, 2022

Thanks for all the work on this!

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.

None yet

2 participants