-
Notifications
You must be signed in to change notification settings - Fork 275
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
API Cleanup 2 #238
Conversation
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>
fn $name(self) -> StyledObject< &'static str> { | ||
StyledObject { | ||
object_style: ObjectStyle { | ||
$side: Some($color), | ||
.. ObjectStyle::default() |
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.
To my opinion it is more clear if we use ObjectStyle, especially in macros where type inspection with idea is a pain.
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.
Kind of disagree here. It's a standard pattern. See the documentation and an example:
fn main() {
let options = SomeOptions { foo: 42, ..Default::default() };
}
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.
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.
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.
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.
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.
Ouch, merged while I was typing. I assume that there's no need to change it once it is already merged.
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.
No, I am fine with it. Isn't that big of a deal. Otherwise, I wouldn't have merged it.
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
Signed-off-by: Robert Vojta <rvojta@me.com>
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.
Looks good, there is no blocker
Take this one as a Cleanup 2 rather than API Cleanup 2.
Changes
tests
moduleErrorKind::source
wasn't returning all error sourcesFrom
forErrorKind
From<&'a str>
&From<String>
forColor
FromStr
Color
tests addedResult
fromget_available_color_count
& simplifiedmacros
reformatting & cleanupObjectStyle
cleanup (deriveDefault
, etc.)mod test
->mod tests
)#[inline]
replaced with#[inline(always)]
AsyncReader::stop_reading()
renamed tostop()
RawScreen::disable_raw_mode_on_drop
renamed tokeep_raw_mode_on_drop
unwrap()s
unwrap()
&expect()
from everywhereunwrap()
calls is thecrossterm_input