Increase the output color of search results by heisen-li · Pull Request #10116 · rust-lang/cargo

heisen-li · 2021-11-24T01:12:36Z

close #9918

rust-highfive · 2021-11-24T01:12:40Z

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

heisen-li · 2021-11-24T09:39:30Z

Why the test task has not passed? I'm still looking for the reason. If anyone knows, feel free to let me know.

ehuss · 2021-11-24T15:52:39Z

Why the test task has not passed?

status() goes to stderr, it can't be used for displaying the search results to stdout.

heisen-li · 2021-11-25T07:44:24Z

Why the test task has not passed?

status() goes to stderr, it can't be used for displaying the search results to stdout.

thanks. I didn't notice, I was too careless. The failed test now seems to have nothing to do with me or something.

ehuss · 2021-11-25T22:21:27Z

Sorry if I wasn't clear. The tests can't change to check stderr, the code will need to be changed to use stdout. I don't think the status function can be used here.

I opened #10120 to track the spurious CI error.

heisen-li · 2021-11-26T02:58:30Z

Thanks, you are right.
The two are indeed different, and the test code cannot be changed rashly.

Currently there is an output method for stderr, but there is no corresponding method for stdout. As a supplement, I added a corresponding output method for stdout.

ehuss · 2021-12-14T18:28:49Z

We talked about this a bit, and felt like coloring the entire entry in bright green can make it a bit difficult to read and doesn't particularly improve the results. We were thinking maybe it could just highlight the matching terms in a normal green color in a fashion similar to npm search. For example cargo search foo would highlight the term foo wherever it appears in the line.

Would you be willing to try that out? It will require a bit more work to figure out a good API for the Shell. These aren't "status" messages precisely. I don't know what that API would look like. I think it would be good to be somewhat flexible (being able to pass a string with specific parts specified with color), but I don't want to make it too complex. For example, there are crates like colored which make it easier, but I would prefer to not bring in a crate just for this. If there is a good balance of simple and flexible, that would be good.

heisen-li · 2021-12-20T10:29:06Z

thank you. I have made the latest changes. I haven't figured out how to design test cases. However, I don't see any other commands to test the output color. It seems unnecessary.

alexcrichton · 2021-12-20T15:30:32Z

Can you post a screenshot or two of what the search results look like?

heisen-li · 2021-12-21T02:38:37Z

thank you, of course.

src/cargo/core/shell.rs

alexcrichton · 2021-12-22T16:25:05Z

src/cargo/core/shell.rs

+        desc_no_query: Vec<&str>,
+        query: &str,
+        color: Color,
+        justified: bool,


This is always true right now so does this need to be a parameter?

Because there are cases where color output is not supported.

I learned the way of the message_stderr()function.

alexcrichton · 2021-12-22T16:25:23Z

src/cargo/core/shell.rs

+    pub fn status_stdout_part_green(
+        &mut self,
+        name_no_query: Vec<&str>,
+        desc_no_query: Vec<&str>,


Can you document what these arguments are? I can't figure out what these are from the signature/name of the method...

Added explanation and changed parameter naming.

src/cargo/core/shell.rs

alexcrichton · 2021-12-22T16:26:42Z

src/cargo/core/shell.rs

+
+                if !desc_no_query.is_empty() {
+                    let mut count_desc = 0;
+                    for message in desc_no_query.iter() {


This looks to be the same loop as above, so can the code be refactored to avoid this duplication?

thank you，refactored the code.

heisen-li · 2022-01-05T01:10:20Z

@ehuss @alexcrichton @Eh2406 Do you mind having a look when you get a chance? I want to know if I am on the right track. Thanks!

alexcrichton · 2022-01-07T16:04:30Z

Sorry I do not personally have time to shepherd along this PR.

ehuss · 2022-01-11T18:08:52Z

@qiangheisenberg Sorry for the lack of direction, but I don't think this is quite the way I was thinking. The way this is written seems to be highly specific to the needs for the search results. I was expecting something that was a little more generic that could be used by other things in the future.

For example, I think the Shell method could be something like write_stdout(text: &str, color: ColorSpec) -> CargoResult<()>. Then the search code could split the string on the matched text, and alternate between normal and green colors. There shouldn't be anything related to justification or status messages in this low-level API.

I also want to say that these reviews are taking an outsized amount of time from us. I would appreciate a little more effort on your side to understand how to approach these problems, and to respond to review comments. Alex asked why message_stdout takes a justified parameter if it is always true. The response to that was not clear, and I also don't see why it took that parameter, as I also see it always taking a true value. I also find the API in this PR a little hard to follow, and the method parameter names are not enough to make that clear.

Finally, I would expect some kind of test to validate the behavior with color. That can be done by using --color=always, and there are a few examples in the testsuite doing that.

This supersedes rust-lang#10116 Fixes rust-lang#9918

epage · 2022-02-26T02:26:06Z

I've gone ahead and created #10425 that tries to resolve @ehuss's concerns as part of an experiment for more flexible colored output for cargo-add. It works alright in this case.

feat(search): Highlight search term This supersedes #10116. For the requested colored-output tests, this followed the pattern of the `fix` tests which just detects whether colored output is used or not. The `cache_messages` actually verify the output is colored but that is because it can just compare to a rustc call's output. Getting the colored output correct by hand in a test (with all of the resets) is a bit messy and would be brittle. This was done in an exercise in exploring ways to generalize colored output support in preparation for `cargo-add` doing some colored output as well. I converted all output calls to use this approach, even if coloring wasn't used, for consistency. I considered coloring the overflow message but decided to hold off on that for now (either a warning-yellow or a hint-gray). Fixes #9918

alexcrichton · 2022-02-28T17:16:07Z

I've r+'d #10425, so I'm going to close this.

heisen-li added 2 commits November 24, 2021 09:02

search color

45aa206

fmt

877b339

rust-highfive assigned Eh2406 Nov 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 24, 2021

heisen-li added 2 commits November 24, 2021 09:17

fix private to public

aa668b4

fix test

971618e

heisen-li force-pushed the search_color branch from f9ca13e to 971618e Compare November 24, 2021 09:37

fix test

742710c

ehuss mentioned this pull request Nov 25, 2021

Unexpected test failure on CI for scrape_examples_basic #10120

Closed

heisen-li force-pushed the search_color branch from d5e768b to c322a72 Compare November 26, 2021 01:59

chang code to use stdout

8682ec6

heisen-li force-pushed the search_color branch from c322a72 to 8682ec6 Compare November 26, 2021 02:21

heisen-li added 2 commits December 20, 2021 16:22

Color the query content

bd82153

fix test

1c8354c

heisen-li force-pushed the search_color branch from 699c337 to 1c8354c Compare December 20, 2021 09:25

alexcrichton reviewed Dec 22, 2021

View reviewed changes

rebuild code

ce0b6ef

heisen-li requested a review from alexcrichton December 25, 2021 06:13

ehuss assigned ehuss and unassigned Eh2406 Jan 11, 2022

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2022

epage added a commit to epage/cargo that referenced this pull request Feb 26, 2022

feat(search): Highlight search term

1eb8913

This supersedes rust-lang#10116 Fixes rust-lang#9918

epage mentioned this pull request Feb 26, 2022

feat(search): Highlight search term #10425

Merged

alexcrichton closed this Feb 28, 2022

Conversation

heisen-li commented Nov 24, 2021

Uh oh!

rust-highfive commented Nov 24, 2021

Uh oh!

heisen-li commented Nov 24, 2021

Uh oh!

ehuss commented Nov 24, 2021

Uh oh!

heisen-li commented Nov 25, 2021

Uh oh!

ehuss commented Nov 25, 2021

Uh oh!

heisen-li commented Nov 26, 2021

Uh oh!

ehuss commented Dec 14, 2021

Uh oh!

heisen-li commented Dec 20, 2021

Uh oh!

alexcrichton commented Dec 20, 2021

Uh oh!

heisen-li commented Dec 21, 2021

Uh oh!

Uh oh!

alexcrichton Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

heisen-li Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

alexcrichton Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

heisen-li Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexcrichton Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

heisen-li Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

heisen-li commented Jan 5, 2022

Uh oh!

alexcrichton commented Jan 7, 2022

Uh oh!

ehuss commented Jan 11, 2022

Uh oh!

epage commented Feb 26, 2022

Uh oh!

alexcrichton commented Feb 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

heisen-li Dec 23, 2021 •

edited

Loading