Skip to content

Document the relationship between the core node and element types - #323

Open
ChrisJr404 wants to merge 2 commits into
rust-scraper:masterfrom
ChrisJr404:document-core-types
Open

Document the relationship between the core node and element types#323
ChrisJr404 wants to merge 2 commits into
rust-scraper:masterfrom
ChrisJr404:document-core-types

Conversation

@ChrisJr404

Copy link
Copy Markdown

Closes #210. Adds a short overview in the crate docs explaining how Html, Node, node::Element, ElementRef and the re-exported Element trait relate and how you move between them, with a runnable example, and clarifies the node::Element doc so it is not confused with the selectors Element trait.

Comment thread scraper/src/node.rs Outdated
pub type Attributes = Vec<(QualName, StrTendril)>;

/// An HTML element.
/// The name and attributes of an HTML element.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While technically correct, this is still the representation of "an HTML element" we use in this crate, not a deliberate subset of name and attributes, so "An HTML element" seems more appropriate conceptually IMO.

Comment thread scraper/src/node.rs Outdated
Comment on lines +225 to +226
/// To get one from a selected element, call
/// [`ElementRef::value`](crate::ElementRef::value).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// To get one from a selected element, call
/// [`ElementRef::value`](crate::ElementRef::value).
/// To access the underlying element of a selected element, call
/// [`ElementRef::value`](crate::ElementRef::value).

Comment thread scraper/src/lib.rs Outdated
//! // The element data: its tag name and attributes.
//! let name = element.value().name();
//! let id = element.value().id();
//! // The text nodes below this element, concatenated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is useful to highlight that this matches all descendant text nodes, e.g. by using <li>two <em>three</em></li> as the second list item.

Comment thread scraper/src/lib.rs Outdated

//! # The core types
//!
//! Parsing an input produces an [`Html`] document that owns a tree of

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
//! Parsing an input produces an [`Html`] document that owns a tree of
//! Parsing produces an [`Html`] document that owns a tree of

"input" does not really add anything.

Comment thread scraper/src/lib.rs Outdated
//! Parsing an input produces an [`Html`] document that owns a tree of
//! [`Node`]s. Every node is one variant of the [`Node`] enum, for example
//! [`Node::Text`] for text and [`Node::Element`] for an element. The data that
//! belongs to an element node, its name and its attributes, is held in a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please try to avoid specifying the data "which makes up an element" as this could change in the future. Prefer a style that names examples if you want to keep them as hints, e.g. "that belongs to an element node, like its attributes, is held in".

Comment thread scraper/src/lib.rs Outdated
//! [`node::Element`](crate::node::Element).
//!
//! Running a [`Selector`] over a document does not hand back bare [`Node`]s.
//! It yields [`ElementRef`]s, each one a handle to an element node that also

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe expand this to explicitly say that CSS selectors can only match elements?

Comment thread scraper/src/lib.rs
//! let selector = Selector::parse("li").unwrap();
//!
//! for element in document.select(&selector) {
//! // The element data: its tag name and attributes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also note that the preferred/short-hand way to access attributes, is ElementRef::attr which might warrant mentioning it here.

Comment thread scraper/src/lib.rs Outdated
//! The re-exported [`Element`] trait is a different thing from
//! [`node::Element`](crate::node::Element). The trait comes from the
//! `selectors` crate and is what lets an [`ElementRef`] be matched against a
//! CSS selector. Most code never needs to name it directly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While true, the most common usage I see is for convenience methods like parent_element and so on provided by the trait impl which I think is also why we have the re-export.

@ChrisJr404

Copy link
Copy Markdown
Author

Thanks for the detailed read. Pushed a revision:

  • node docs now open with "An HTML element" and use the "access the underlying element" wording.
  • Dropped "an input" and stopped enumerating an element's data as name plus attributes, using attributes as an example instead.
  • Called out that CSS selectors only match elements, and that the re-exported trait is what provides convenience methods like parent_element on ElementRef.
  • Example now uses ElementRef::attr for the id and adds a nested em to the second list item so text collecting all descendant text is visible.

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.

More explicit description to Element type and some doc complement

2 participants