alamb commented on code in PR #1549:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1859075112
##########
src/ast/spans.rs:
##########
@@ -21,21 +21,51 @@ use super::{
/// Given an iterator of spans, return the [Span::union] of all spans.
fn union_spans<I: Iterator<Item = Span>>(iter: I) -> Span {
- iter.reduce(|acc, item| acc.union(&item))
- .unwrap_or(Span::empty())
+ Span::union_iter(iter)
}
-/// A trait for AST nodes that have a source span for use in diagnostics.
+/// Trait for AST nodes that have a source location information.
///
-/// Source spans are not guaranteed to be entirely accurate. They may
-/// be missing keywords or other tokens. Some nodes may not have a computable
-/// span at all, in which case they return [`Span::empty()`].
+/// # Notes:
+///
+/// Source [`Span`] are not yet complete. They may be missing:
+///
+/// 1. keywords or other tokens
+/// 2. span information entirely, in which case they return [`Span::empty()`].
+///
+/// Note Some impl blocks (rendered below) are annotated with which nodes are
+/// missing spans. See [this ticket] for additional information and status.
+///
+/// [this ticket]:
https://github.com/apache/datafusion-sqlparser-rs/issues/1548
+///
+/// # Example
Review Comment:
I felt an example will help people understand how to use the Spans
##########
src/ast/helpers/attached_token.rs:
##########
@@ -80,3 +128,9 @@ impl From<TokenWithLocation> for AttachedToken {
AttachedToken(value)
}
}
+
+impl From<AttachedToken> for TokenWithLocation {
Review Comment:
this seemed to be an obvious gap so I just added it to avoid a papercut
##########
src/lib.rs:
##########
@@ -61,13 +64,67 @@
//! // The original SQL text can be generated from the AST
//! assert_eq!(ast[0].to_string(), sql);
//! ```
-//!
//! [sqlparser crates.io page]: https://crates.io/crates/sqlparser
//! [`Parser::parse_sql`]: crate::parser::Parser::parse_sql
//! [`Parser::new`]: crate::parser::Parser::new
//! [`AST`]: crate::ast
//! [`ast`]: crate::ast
//! [`Dialect`]: crate::dialect::Dialect
+//!
+//! # Source Spans
Review Comment:
This content is largely migrated from `docs/source_spans.md`
##########
src/tokenizer.rs:
##########
@@ -448,10 +470,25 @@ impl fmt::Debug for Location {
}
impl Location {
- pub fn of(line: u64, column: u64) -> Self {
+ /// Return an "empty" / unknown location
+ pub fn empty() -> Self {
+ Self { line: 0, column: 0 }
+ }
+
+ /// Create a new `Location` for a given line and column
+ pub fn new(line: u64, column: u64) -> Self {
Self { line, column }
}
+ /// Create a new location for a given line and column
+ ///
+ /// Alias for [`Self::new`]
+ // TODO: remove / deprecate in favor of` `new` for consistency?
Review Comment:
I found the `Location::of` API kind of confusing -- I would expect that
creating a new Rust object would be done via `::new()` so I added a `new()`
I am thinking perhaps we can deprecate `Location::of` maybe to help
downstream users
##########
src/tokenizer.rs:
##########
@@ -422,13 +422,35 @@ impl fmt::Display for Whitespace {
}
/// Location in input string
+///
+/// # Create an "empty" (unknown) `Location`
+/// ```
+/// # use sqlparser::tokenizer::Location;
+/// let location = Location::empty();
+/// ```
+///
+/// # Create a `Location` from a line and column
+/// ```
+/// # use sqlparser::tokenizer::Location;
+/// let location = Location::new(1, 1);
+/// ```
+///
+/// # Create a `Location` from a pair
+/// ```
+/// # use sqlparser::tokenizer::Location;
+/// let location = Location::from((1, 1));
+/// ```
#[derive(Eq, PartialEq, Hash, Clone, Copy, Ord, PartialOrd)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub struct Location {
- /// Line number, starting from 1
+ /// Line number, starting from 1.
Review Comment:
`///` means these comments show up in docs
##########
src/ast/spans.rs:
##########
@@ -21,21 +21,51 @@ use super::{
/// Given an iterator of spans, return the [Span::union] of all spans.
fn union_spans<I: Iterator<Item = Span>>(iter: I) -> Span {
- iter.reduce(|acc, item| acc.union(&item))
- .unwrap_or(Span::empty())
+ Span::union_iter(iter)
Review Comment:
this was useful enough I felt making it a method in `Span` with more
documentation and examples would make it easier to find
##########
src/tokenizer.rs:
##########
@@ -512,16 +567,61 @@ impl Span {
}
/// Same as [Span::union] for `Option<Span>`
+ ///
/// If `other` is `None`, `self` is returned
pub fn union_opt(&self, other: &Option<Span>) -> Span {
match other {
Some(other) => self.union(other),
None => *self,
}
}
+
+ /// Return the [Span::union] of all spans in the iterator
+ ///
+ /// If the iterator is empty, an empty span is returned
+ ///
+ /// # Example
+ /// ```
+ /// # use sqlparser::tokenizer::{Span, Location};
+ /// let spans = vec![
+ /// Span::new(Location::new(1, 1), Location::new(2, 5)),
+ /// Span::new(Location::new(2, 3), Location::new(3, 7)),
+ /// Span::new(Location::new(3, 1), Location::new(4, 2)),
+ /// ];
+ /// // line 1, column 1 -> line 4, column 2
+ /// assert_eq!(
+ /// Span::union_iter(spans),
+ /// Span::new(Location::new(1, 1), Location::new(4, 2))
+ /// );
+ pub fn union_iter<I: IntoIterator<Item = Span>>(iter: I) -> Span {
Review Comment:
This was a private function and I thought it was useful enough it should be
its own API and have an example, so I pulled it into a method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
