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.

Sign up for GitHub

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

Jump to bottom

Finished color refactor #1824

Merged
merged 1 commit into from Apr 16, 2020
Merged

Finished color refactor #1824

merged 1 commit into from Apr 16, 2020

Conversation

pksunkara
Copy link
Member

No description provided.

@pksunkara pksunkara linked an issue Apr 14, 2020 that may be closed by this pull request
Refactor color support (+Add Windows Support) #836
Closed
This was referenced Apr 14, 2020
Open
Closed
Closed
CreepySkeleton
CreepySkeleton reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

This looks good so far, but I'd like to test it on Windows once you make it build to make sure it actually does work.

Do we really want to make the coloring facilities public? Maybe limit this API to Error and ColorChoice?

@CreepySkeleton
Copy link
Contributor

Another thought: do we really need to panic if coloring fails? Maybe just ignore the error and try to write uncolored?

@pksunkara pksunkara force-pushed the color branch 3 times, most recently from 6f8c2ba to 529f863 Compare April 15, 2020 10:06
@pksunkara
Copy link
Member Author

This is ready. I haven't made them public at all. Also panics are io::write default stuff.

CreepySkeleton
CreepySkeleton suggested changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

Feel free to address/ignore to_string spam, my concerns are:

  • fn exit(&mut self) -> ! - no real need for it to be mut
  • pub where it doesn't belong - or does it?
  • Panicking on failed set_color

examples/13a_enum_values_automatic.rs Outdated Show resolved Hide resolved
examples/12_typed_values.rs Outdated Show resolved Hide resolved
examples/13b_enum_values_manual.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Outdated Show resolved Hide resolved
tests/cargo.rs Outdated Show resolved Hide resolved
tests/cargo.rs Outdated Show resolved Hide resolved
tests/version.rs Outdated Show resolved Hide resolved
tests/version.rs Outdated Show resolved Hide resolved
src/output/fmt.rs Show resolved Hide resolved
@pksunkara pksunkara force-pushed the color branch 3 times, most recently from 55dde33 to 8df109b Compare April 16, 2020 08:01
@pksunkara pksunkara mentioned this pull request Apr 16, 2020
Merged
CreepySkeleton
CreepySkeleton suggested changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

Almost ready 😛

src/parse/matches/arg_matches.rs Outdated
@@ -331,17 +331,21 @@ impl ArgMatches {
pub fn value_of_t<R>(&self, name: &str) -> Result<R, Error>
where
R: FromStr,
<R as FromStr>::Err: Display,
<R as FromStr>::Err: Display + Debug,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need Debug here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of parsed.expect below.

src/parse/matches/arg_matches.rs Outdated
Comment on lines 339 to 346
if let Err(e) = parsed {
Err(Error::value_validation_auto(&format!(
"The argument '{}' isn't a valid value: {}",
v, e
))
})
))?)
} else {
Ok(parsed.expect(INTERNAL_ERROR_MSG))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use match instead of if let Err {} else { unwrap }.

Btw, you can just .map_err here.

Copy link
Member Author

Choose a reason for hiding this comment

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

map_err doesn't work because Error::value_validation_auto returns a Result and using the ? gives:

the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `std::ops::Try`)

Copy link
Contributor

Choose a reason for hiding this comment

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

Than use Result::or_else which is made exactly for this situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of think this situation when creating an error - not displaying! - can return io::Error is rather unfortunate. I''' be seeing into this further, but not blocker for this PR.

src/parse/matches/arg_matches.rs Outdated
@@ -378,7 +382,7 @@ impl ArgMatches {
pub fn value_of_t_or_exit<R>(&self, name: &str) -> R
where
R: FromStr,
<R as FromStr>::Err: Display,
<R as FromStr>::Err: Display + Debug,
Copy link
Contributor

Choose a reason for hiding this comment

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

Another debug

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of forwarding it to value_of_t

src/parse/matches/arg_matches.rs Outdated
Comment on lines 429 to 436
if let Err(e) = parsed {
Err(Error::value_validation_auto(&format!(
"The argument '{}' isn't a valid value: {}",
v, e
))
})
))?)
} else {
Ok(parsed.expect(INTERNAL_ERROR_MSG))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yet another candidate for map_err

Copy link
Member Author

Choose a reason for hiding this comment

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

Explained the reason above

src/parse/matches/arg_matches.rs Show resolved Hide resolved
src/parse/matches/arg_matches.rs Outdated
@@ -466,7 +474,7 @@ impl ArgMatches {
pub fn values_of_t_or_exit<R>(&self, name: &str) -> Vec<R>
where
R: FromStr,
<R as FromStr>::Err: Display,
<R as FromStr>::Err: Display + Debug,
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of forwarding to values_of_t and because values_of_t has parsed.expect

@CreepySkeleton
Copy link
Contributor

It doesn't play nicely with cargo run

image

Can we ensure that we always end the message with at least one LF \n?

CreepySkeleton
CreepySkeleton approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 16, 2020

Build succeeded:

@bors bors bot merged commit 4c3a7f7 into master Apr 16, 2020
@bors bors bot deleted the color branch April 16, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CreepySkeleton CreepySkeleton CreepySkeleton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor color support (+Add Windows Support)
2 participants
@pksunkara @CreepySkeleton