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

API Cleanup 2 #238

Merged
merged 30 commits into from
Sep 19, 2019
Merged

API Cleanup 2 #238

merged 30 commits into from
Sep 19, 2019

Conversation

zrzka
Copy link
Contributor

@zrzka zrzka commented Sep 19, 2019

Take this one as a Cleanup 2 rather than API Cleanup 2.

Changes

  • Some tests weren't in a separate tests module
  • ErrorKind::source wasn't returning all error sources
  • New macro to implement From for ErrorKind
  • Removed redundant From<&'a str> & From<String> for Color
    • If you have both, you can use FromStr
  • Some Color tests added
  • Removed Result from get_available_color_count & simplified
  • Style macros reformatting & cleanup
  • Style ObjectStyle cleanup (derive Default, etc.)
  • Some docstring examples tests fixed & reenabled
  • Sync/fix tests modules (mod test -> mod tests)
  • Added missing empty lines between fns
  • #[inline] replaced with #[inline(always)]
    • The intention was to always inline, right?
    • Reference
    • I'd keep this decision on the compiler and would probably remove it completely
  • AsyncReader::stop_reading() renamed to stop()
  • RawScreen::disable_raw_mode_on_drop renamed to keep_raw_mode_on_drop
    • It was a wrong name, with an opposite meaning
  • Other various reformatting & cleanup

unwrap()s

  • Removed unwrap() & expect() from everywhere
  • The only crate with unwrap() calls is the crossterm_input
    • Not going to solve it in this PR, because I have to think about it as there's threading, channels, ... involved

Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
This reverts commit e1e4907.

Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
crossterm_cursor/src/cursor/ansi_cursor.rs Outdated Show resolved Hide resolved
crossterm_screen/src/screen/raw.rs Show resolved Hide resolved
crossterm_screen/src/sys.rs Show resolved Hide resolved
crossterm_style/src/lib.rs Outdated Show resolved Hide resolved
fn $name(self) -> StyledObject< &'static str> {
StyledObject {
object_style: ObjectStyle {
$side: Some($color),
.. ObjectStyle::default()
Copy link
Member

Choose a reason for hiding this comment

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

To my opinion it is more clear if we use ObjectStyle, especially in macros where type inspection with idea is a pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of disagree here. It's a standard pattern. See the documentation and an example:

fn main() {
    let options = SomeOptions { foo: 42, ..Default::default() };
}

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I can find my way in. In this case, we have a macro for which we don't have a type of inspection or highlighting for a lot of IDEs. ObjectStyle::default()is much more specific than Deafault::default()`, so someone who reads this macro can better understand what it says. So it's more about readability and being specific in macros.

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 macro looks like this ...

StyledObject {
    object_style: ObjectStyle { // <-- here
        $side: Some($color),
        ..Default::default()
    },
    content: self
}

... and you have the ObjectStyle 2 lines above the ..Default::default(), which looks enough & fine to me.

There's also a similar case in the macro below (def_str_attr) ...

StyledObject {
    object_style: Default::default(),
    content: self,
}

... where Default::default() is used, but there's no ObjectStyle at all. Which is, from the readability point of view a bit worse than the first case.

I do not want to force my opinion here, both ways can be used, so, I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, merged while I was typing. I assume that there's no need to change it once it is already merged.

Copy link
Member

@TimonPost TimonPost Sep 19, 2019

Choose a reason for hiding this comment

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

No, I am fine with it. Isn't that big of a deal. Otherwise, I wouldn't have merged it.

crossterm_utils/src/error.rs Outdated Show resolved Hide resolved
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Copy link
Member

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

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

Looks good, there is no blocker

@TimonPost TimonPost merged commit 4952cb3 into crossterm-rs:master Sep 19, 2019
@zrzka zrzka deleted the zrzka/api-cleanup-2 branch September 19, 2019 15:57
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