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

Allow creating a Term from an arbitrary Read/Write pair. #93

Merged

Conversation

stuhood
Copy link
Contributor

@stuhood stuhood commented Feb 10, 2021

Fixes #34. Method and struct names are eminently bikesheddable.

Unfortunately there are some public API changes necessary: described inline.

/// Where the term is writing.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[derive(Debug, Clone)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The derivation of Copy just isn't possible in a safe way for arbitrary Read/Write destinations. And allowing for equality checks is of questionable value, although we can add it back if need be.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add it back to maintain as much backward compatibility as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy cannot be added back, since no useful file handles are Copy (without unsafe).

Eq/PartialEq might be possible to add back, but it would need a strange/custom implementation (ie, assigning the Term a unique id?), because neither of those traits is object safe:

error[E0038]: the trait `TermWrite` cannot be made into an object
  --> src/term.rs:28:22
   |
28 |     write: Arc<Mutex<dyn TermWrite>>,
   |                      ^^^^^^^^^^^^^ `TermWrite` cannot be made into an object
   |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
  --> src/term.rs:15:44
   |
15 | trait TermWrite: Write + Debug + AsRawFd + PartialEq + Send {}
   |       ---------                            ^^^^^^^^^ ...because it uses `Self` as a type parameter
   |       |
   |       this trait cannot be made into an object...

src/term.rs Outdated Show resolved Hide resolved
@stuhood stuhood force-pushed the stuhood/term-target-for-arbitrary-handles branch from c016300 to a29180f Compare February 10, 2021 21:50
@stuhood

This comment has been minimized.

src/term.rs Outdated Show resolved Hide resolved
@stuhood stuhood force-pushed the stuhood/term-target-for-arbitrary-handles branch from b6e9aa7 to 76bc0a7 Compare March 21, 2021 21:14
@stuhood
Copy link
Contributor Author

stuhood commented Mar 21, 2021

@pksunkara : Added an optional Style.

Also, the names of structs/methods continue to be completely open to change if you have any thoughts there.

src/term.rs Outdated Show resolved Hide resolved
/// Where the term is writing.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[derive(Debug, Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add it back to maintain as much backward compatibility as possible.

src/term.rs Outdated Show resolved Hide resolved
@pksunkara pksunkara merged commit 3232775 into console-rs:master Mar 23, 2021
@stuhood stuhood deleted the stuhood/term-target-for-arbitrary-handles branch March 23, 2021 19:30
@stuhood stuhood restored the stuhood/term-target-for-arbitrary-handles branch March 23, 2021 19:30
@stuhood stuhood deleted the stuhood/term-target-for-arbitrary-handles branch March 23, 2021 19:31
dtolnay pushed a commit to dtolnay-contrib/console that referenced this pull request Feb 4, 2022
This branch updates the `console-subscriber` env var parsing code to accept
durations with units. Durations may now be given in any of `ns`, `us`,
`ms`, `s`, `sec`, `seconds`, `m`, `min`, `minutes`, `h`, or `hours`.
Durations specified in seconds or a larger unit may also be given as fractional values,
such as `1.5min`.

As a follow-up, we may also want to allow specifying durations as a
combination of multiple units, such as `3h15m`.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.

Create a Term from arbitrary Read and Write pair
2 participants