iffyio commented on code in PR #1549: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1549#discussion_r1860722501
########## 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 +//! +//! Starting with version `0.53.0` sqlparser introduced source spans to the +//! AST. This feature provides source information for syntax errors enables +//! better error messages. See [issue #1548] for more information and the +//! [`Spanned`] traite to access the spans. +//! +//! [issue #1548]: https://github.com/apache/datafusion-sqlparser-rs/issues/1548 +//! [`Spanned`]: ast::Spanned +//! +//! ## Migration Guide +//! +//! For the next few releases, we will be incrementally adding source spans to the +//! AST odes, trying to minimize the impact on existing users. Some breaking Review Comment: ```suggestion //! AST nodes, trying to minimize the impact on existing users. Some breaking ``` ########## src/ast/mod.rs: ########## @@ -596,9 +596,21 @@ pub enum CeilFloorKind { /// An SQL expression of any type. /// +/// # Semantics / Type Checking +/// /// The parser does not distinguish between expressions of different types -/// (e.g. boolean vs string), so the caller must handle expressions of -/// inappropriate type, like `WHERE 1` or `SELECT 1=1`, as necessary. +/// (e.g. boolean vs string). The the caller is responsible for detecting and Review Comment: ```suggestion /// (e.g. boolean vs string). The caller is responsible for detecting and ``` ########## 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: That sounds reasonable to me! ########## 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 +//! +//! Starting with version `0.53.0` sqlparser introduced source spans to the +//! AST. This feature provides source information for syntax errors enables Review Comment: ```suggestion //! AST. This feature provides source information for syntax errors, enabling ``` not sure if the edit does justice to the intended sentence, the 'enables' part seemed to read off was why ########## 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 +//! +//! Starting with version `0.53.0` sqlparser introduced source spans to the +//! AST. This feature provides source information for syntax errors enables +//! better error messages. See [issue #1548] for more information and the +//! [`Spanned`] traite to access the spans. Review Comment: ```suggestion //! [`Spanned`] trait to access the spans. ``` -- 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]
