Update comments / docs for `Spanned` #1549

alamb · 2024-11-25T12:24:29Z

I took a pass over the code in #1435 and found some places
I thought adding some more documentation would be helpful.

Changes

Updates rustdocs, and adds a bunch of examples
Adds links to [EPIC] Complete Span (source location) information / feature #1548

Follow on work (tickets)

alamb · 2024-11-25T15:48:17Z

docs/source_spans.md

@@ -1,52 +0,0 @@
-


This content is great. However, I think it may be hard to find given it is in a separate file

I moved the content into two places:

docs in lib.rs for stuff that is relevant to users

The [EPIC] Complete Span (source location) information / feature #1548 for things that are related to contributors

I think that will make it more likely the documentation can be discovered

alamb · 2024-11-26T18:44:44Z

README.md

-syntax from newer versions that have been explicitly requested, plus some MSSQL,
-PostgreSQL, and other dialect-specific syntax. Whenever possible, the [online
-SQL:2016 grammar][sql-2016-grammar] is used to guide what syntax to accept.
+syntax from newer versions that have been explicitly requested, plus various


drive by clarification

alamb · 2024-11-26T18:45:48Z

src/ast/helpers/attached_token.rs

@@ -80,3 +128,9 @@ impl From<TokenWithLocation> for AttachedToken {
        AttachedToken(value)
    }
 }
+
+impl From<AttachedToken> for TokenWithLocation {


this seemed to be an obvious gap so I just added it to avoid a papercut

alamb · 2024-11-26T18:47:35Z

src/ast/spans.rs

+///
+/// [this ticket]: https://github.com/apache/datafusion-sqlparser-rs/issues/1548
+///
+/// # Example


I felt an example will help people understand how to use the Spans

alamb · 2024-11-26T18:47:56Z

src/lib.rs

 //! [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


This content is largely migrated from docs/source_spans.md

alamb · 2024-11-26T18:48:16Z

src/tokenizer.rs

 #[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.


/// means these comments show up in docs

alamb · 2024-11-26T18:49:28Z

src/tokenizer.rs

+    /// Create a new location for a given line and column
+    ///
+    /// Alias for [`Self::new`]
+    // TODO: remove / deprecate in favor of` `new` for consistency?


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

That sounds reasonable to me!

alamb · 2024-11-26T18:50:39Z

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)


this was useful enough I felt making it a method in Span with more documentation and examples would make it easier to find

alamb · 2024-11-26T18:50:46Z

src/tokenizer.rs

+    ///   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 {


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

iffyio

LGTM! Thanks for the updates, the docs look great!

src/ast/mod.rs

src/lib.rs

iffyio · 2024-11-27T14:09:59Z

src/tokenizer.rs

+    /// Create a new location for a given line and column
+    ///
+    /// Alias for [`Self::new`]
+    // TODO: remove / deprecate in favor of` `new` for consistency?


That sounds reasonable to me!

Co-authored-by: Ifeanyi Ubah <[email protected]>

alamb · 2024-11-27T16:39:49Z

Thanks @iffyio and @lovasoa -- I'll plan to leave this open for a few more days in case anyone else wants a chance to comment

alamb · 2024-11-30T13:08:53Z

🚀
Let's get this in

alamb · 2024-11-30T13:09:00Z

Thanks again @iffyio and @lovasoa

alamb marked this pull request as draft November 25, 2024 12:24

alamb force-pushed the alamb/doc_sql_parser branch from 2092351 to bb7ad71 Compare November 25, 2024 15:18

alamb commented Nov 25, 2024

View reviewed changes

Update README.md, add docs + examples

ec24d26

alamb force-pushed the alamb/doc_sql_parser branch from d08e6ec to ec24d26 Compare November 26, 2024 18:43

alamb commented Nov 26, 2024

View reviewed changes

alamb marked this pull request as ready for review November 26, 2024 18:50

This was referenced Nov 26, 2024

Implement Spanned to retrieve source locations on AST nodes #1435

Merged

Rename TokenWithLocation to TokenWithSpan, in backwards compatible way #1562

Merged

Create test pattern for Spans #1563

Open

lovasoa approved these changes Nov 26, 2024

View reviewed changes

iffyio approved these changes Nov 27, 2024

View reviewed changes

Apply suggestions from code review

018afff

Co-authored-by: Ifeanyi Ubah <[email protected]>

alamb added 2 commits November 30, 2024 08:00

Merge remote-tracking branch 'apache/main' into alamb/doc_sql_parser

19a9278

Merge remote-tracking branch 'apache/main' into alamb/doc_sql_parser

dd9cfee

alamb merged commit 4ab3ab9 into apache:main Nov 30, 2024
8 checks passed

alamb deleted the alamb/doc_sql_parser branch November 30, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update comments / docs for `Spanned` #1549

Update comments / docs for `Spanned` #1549

alamb commented Nov 25, 2024 •

edited

Loading

alamb Nov 25, 2024 •

edited

Loading

alamb Nov 26, 2024

alamb Nov 26, 2024

alamb Nov 26, 2024

alamb Nov 26, 2024

alamb Nov 26, 2024

alamb Nov 26, 2024

iffyio Nov 27, 2024

alamb Nov 26, 2024

alamb Nov 26, 2024

iffyio left a comment

iffyio Nov 27, 2024

alamb commented Nov 27, 2024 •

edited

Loading

alamb commented Nov 30, 2024

alamb commented Nov 30, 2024

Update comments / docs for Spanned #1549

Update comments / docs for Spanned #1549

Conversation

alamb commented Nov 25, 2024 • edited Loading

alamb Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 27, 2024 • edited Loading

alamb commented Nov 30, 2024

alamb commented Nov 30, 2024

Update comments / docs for `Spanned` #1549

Update comments / docs for `Spanned` #1549

alamb commented Nov 25, 2024 •

edited

Loading

alamb Nov 25, 2024 •

edited

Loading

alamb commented Nov 27, 2024 •

edited

Loading