From 0d518bf8136fd9793dd13c5369ff65288242c682 Mon Sep 17 00:00:00 2001
From: kik4444 <7779637+kik4444@users.noreply.github.com>
Date: Sat, 3 Feb 2024 03:40:47 +0200
Subject: [PATCH] `query web --query` should return `list<list<string>>` like
 the scraper crate's `ElementRef::text()` (#11705)

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
## Problem
I tried converting one of my Rust web scrapers to Nushell just to see
how it would be done, but quickly ran into an issue that proved annoying
to fix without diving into the source.

For instance, let's say we have the following HTML
```html
<p>Hello there, <span style="color: red;">World</span></p>
```
and we want to extract only the text within the `p` element, but not the
`span`. With the current version of nu_plugin_query, if we run this code
```nushell
echo `<p>Hello there, <span style="color: red;">World</span></p>` | query web -q "p" | get 0
# returns "Hello there, World"

# but we want only "Hello there, "
```
we will get back a `list<string>` that contains 1 string `Hello there,
World`.
To avoid scraping the span, we would have to do something like this
```nushell
const html = `<p>Hello there, <span style="color: red;">World</span></p>`
$html
| query web -q "p"
| get 0
| str replace ($html | query web -q "p > span" | get 0) ""
# returns "Hello there, "
```
In other words, we would have to make a sub scrape of the text we
*don't* want in order to subtract it from the text we *do* want.

## Solution
I didn't like this behavior, so I decided to change it. I modified the
`execute_selector_query` function to collect all text nodes in the HTML
element matching the query. Now `query web --query` will return a
`list<list<string>>`
```nushell
echo `<p>Hello there, <span style="color: red;">World</span></p>` | query web -q "p" | get 0 | to json --raw
# returns ["Hello there, ","World"]
```
This also brings `query web --query`'s behavior more in line with
[scraper's
ElementRef::text()](https://docs.rs/scraper/latest/scraper/element_ref/struct.ElementRef.html#method.text)
which "Returns an iterator over descendent text nodes", allowing you to
choose how much of an element's text you want to scrape without
resorting to string substitutions.

## Consequences
As this is a user-facing change, the usage examples will produce
different results than before. For example
```nushell
http get https://phoronix.com | query web --query 'header'
```
will return a list of lists of 1 string each, whereas before it was just
a list of strings.

I only modified the 3rd example
```nushell
# old
http get https://www.nushell.sh | query web --query 'h2, h2 + p' | group 2 | each {rotate --ccw tagline description} | flatten
# new
http get https://www.nushell.sh | query web --query 'h2, h2 + p' | each {str join} | group 2 | each {rotate --ccw tagline description} | flatten
```
to make it behave like before because I thought this one ought to show
the same results as before.
However, the second reason I changed the 3rd example is because it
otherwise panics! If we run the original 3rd example with my
modifications, we get a panic
```
thread 'main' panicked at crates/nu-protocol/src/value/record.rs:34:9:
assertion `left == right` failed
  left: 2
 right: 17
```
This happens because `rotate` receives a list of lists where the inner
lists have a different number of elements.

However this panic is unrelated to the changes I've made, because it can
be triggered easily without using the plugin. For instance
```nushell
# this is fine
[[[one] [two]] [[three] [four]]] | each {rotate --ccw tagline description}

# this panics!
[[[one] [two]] [[three] [four five]]] | each {rotate --ccw tagline description}
```
Though beyond the scope of this PR, I thought I'd mention this bug since
I found it while testing the usage examples. However, I intend to make a
proper issue about it tomorrow.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->
`query web --query "css selector"` now returns a `list<list<string>>`
instead of a `list<string>` to make it more in line with [scraper's
ElementRef::text()](https://docs.rs/scraper/latest/scraper/element_ref/struct.ElementRef.html#method.text).

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
I ran `cargo fmt --all -- --check`, `cargo clippy --workspace -- -D
warnings -D clippy::unwrap_used` and the tests in the plugin.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
PR that updates the documentation to match the new 3rd example:
https://github.com/nushell/nushell.github.io/pull/1235
---
 crates/nu_plugin_query/src/nu/mod.rs    |  4 +--
 crates/nu_plugin_query/src/query_web.rs | 38 +++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/crates/nu_plugin_query/src/nu/mod.rs b/crates/nu_plugin_query/src/nu/mod.rs
index 80c0bf8571..fcd5eaa4c3 100644
--- a/crates/nu_plugin_query/src/nu/mod.rs
+++ b/crates/nu_plugin_query/src/nu/mod.rs
@@ -73,7 +73,7 @@ impl Plugin for Query {
 
 pub fn web_examples() -> Vec<PluginExample> {
     vec![PluginExample {
-        example: "http get https://phoronix.com | query web --query 'header'".into(),
+        example: "http get https://phoronix.com | query web --query 'header' | flatten".into(),
         description: "Retrieve all `<header>` elements from phoronix.com website".into(),
         result: None,
     }, PluginExample {
@@ -83,7 +83,7 @@ pub fn web_examples() -> Vec<PluginExample> {
         result: None
     },
     PluginExample {
-        example: "http get https://www.nushell.sh | query web --query 'h2, h2 + p' | group 2 | each {rotate --ccw tagline description} | flatten".into(),
+        example: "http get https://www.nushell.sh | query web --query 'h2, h2 + p' | each {str join} | group 2 | each {rotate --ccw tagline description} | flatten".into(),
         description: "Pass multiple css selectors to extract several elements within single query, group the query results together and rotate them to create a table".into(),
         result: None,
     },
diff --git a/crates/nu_plugin_query/src/query_web.rs b/crates/nu_plugin_query/src/query_web.rs
index 9318ea9dd7..ad6eca4f97 100644
--- a/crates/nu_plugin_query/src/query_web.rs
+++ b/crates/nu_plugin_query/src/query_web.rs
@@ -260,10 +260,11 @@ fn execute_selector_query(
         false => doc
             .select(&css(query_string, inspect))
             .map(|selection| {
-                Value::string(
+                Value::list(
                     selection
                         .text()
-                        .fold("".to_string(), |acc, x| format!("{acc}{x}")),
+                        .map(|text| Value::string(text, span))
+                        .collect(),
                     span,
                 )
             })
@@ -293,6 +294,8 @@ mod tests {
          </ul>
      "#;
 
+    const NESTED_TEXT: &str = r#"<p>Hello there, <span style="color: red;">World</span></p>"#;
+
     #[test]
     fn test_first_child_is_not_empty() {
         assert!(!execute_selector_query(
@@ -316,6 +319,35 @@ mod tests {
         );
         let config = nu_protocol::Config::default();
         let out = item.into_string("\n", &config);
-        assert_eq!("[Coffee]".to_string(), out)
+        assert_eq!("[[Coffee]]".to_string(), out)
+    }
+
+    #[test]
+    fn test_nested_text_nodes() {
+        let item = execute_selector_query(
+            NESTED_TEXT,
+            "p:first-child",
+            false,
+            false,
+            Span::test_data(),
+        );
+        let out = item
+            .as_list()
+            .unwrap()
+            .iter()
+            .map(|matches| {
+                matches
+                    .as_list()
+                    .unwrap()
+                    .iter()
+                    .map(|text_nodes| text_nodes.as_string().unwrap())
+                    .collect::<Vec<String>>()
+            })
+            .collect::<Vec<Vec<String>>>();
+
+        assert_eq!(
+            out,
+            vec![vec!["Hello there, ".to_string(), "World".to_string()]],
+        );
     }
 }