Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add XPath and XQuery lexers #1089

Merged
merged 26 commits into from
Jul 1, 2019
Merged

Add XPath and XQuery lexers #1089

merged 26 commits into from
Jul 1, 2019

Conversation

MaximeKjaer
Copy link
Contributor

@MaximeKjaer MaximeKjaer commented Feb 8, 2019

This PR adds support for XPath 3.1 and XQuery 3.1. These are W3C recommendation languages made for querying tree-structured data.

From the XQuery documentation:

XQuery 3.1 is an extension of XPath 3.1. In general, any expression that is syntactically valid and executes successfully in both XPath 3.1 and XQuery 3.1 will return the same result in both languages.

Therefore, the XQuery lexer extends the XPath.

I'm new to Ruby (but not to lexers and parsers!), so if there's anything that could be done better, let me know. For instance, I would have wanted to inherit the regex for eqName in the XQuery lexer, but had trouble finding a solution that both allowed me to inherit and use the regex in rules 🤷‍♂️ There are therefore a few regexes that have been duplicated in the XQuery lexer.

I'm keeping the Git history true to the way I wrote these lexers, but I'm happy to rebase, or for the commits to be squashed on merge.

I don't expect CI to pass because of the issue highlighted in #1062.

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label May 28, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 4, 2019

@MaximeKjaer Sorry that it's taken so long to get back to you on this. I'm going through the lexers and will have some comments soon!

lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xquery.rb Show resolved Hide resolved
lib/rouge/lexers/xquery.rb Outdated Show resolved Hide resolved
spec/visual/samples/xpath Outdated Show resolved Hide resolved
@pyrmont
Copy link
Contributor

pyrmont commented Jun 4, 2019

Sorry, there's a lot there! Please don't hesitate to post any questions you might have! Or any corrections to silly things I said :)

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 4, 2019
@MaximeKjaer
Copy link
Contributor Author

Thank you so much for this extensive review 🎉 I'm happy to have learned a bunch while working on this, it's been fun :)

I've replied to all your points individually above. As I said in one of the replies, I rebased to make sure all code examples throughout the commit history are from a permissive copyright license, and used with the appropriate copyright notice.

Rebasing messed a little bit with the displayed ordering of commits on GitHub. so I'm sorry about that 😬 Apparently this is only a GitHub-specific issue; the commits are fine, and are in the correct order -- it's just GitHub displaying them incorrectly on the PR UI. As the help page says:

If you always want to see commits in order, we recommend not using git rebase. However, rest assured that nothing is broken when you see things outside of a chronological order!

@ashmaroli
Copy link
Contributor

ashmaroli commented Jun 15, 2019

@MaximeKjaer In order to reduce warnings issued from the Ruby interpreter, we recommend using the %r syntax for regular expression literals passed as arguments (e.g. to the method :rule). (However, optionally using that syntax everywhere brings consistency).

Also, it'd be better if individual method definitions are separated by a blank line:

      # Terminal literals:
      # https://www.w3.org/TR/xpath-31/#terminal-symbols

      def self.digits
        @digits ||= %r/[0-9]+/
      end

      def self.decimalLiteral
        @decimalLiteral ||= %r/\.#{digits}|#{digits}\.[0-9]*/
      end

      def self.doubleLiteral
        @doubleLiteral ||= %r/(\.#{digits})|#{digits}(\.[0-9]*)?[eE][+-]?#{digits}/
      end
      # Operators
      rule XPath.operators, Operator
      rule %r/\b#{XPath.word_operators}\b/, Operator::Word
      rule %r/\b#{XPath.keywords}\b/, Keyword
      rule %r/[?,{}()\[\]]/, Punctuation

      # Functions
      rule %r/(function)(\s*)(#{XPath.openParen})/ do # function declaration
        groups Keyword, Text::Whitespace, Punctuation
      end

For further details and examples, you may refer to #1180

@MaximeKjaer
Copy link
Contributor Author

@ashmaroli Thank you for the feedback! Spacing is fixed in 64f0276, and %r in 96f2614

@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jun 25, 2019
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jun 25, 2019
Copy link
Contributor

@pyrmont pyrmont left a comment

Choose a reason for hiding this comment

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

Wow, this is really an impressive amount of work. Please let me know if any of my comments don't make sense.

Oh, and I think you're missing a spec for XPath.

lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xpath.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xquery.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xquery.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/xquery.rb Outdated Show resolved Hide resolved
@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 25, 2019
@MaximeKjaer
Copy link
Contributor Author

The code review is a lot of work to, so thank you!

I've addressed all your comments individually, and added an XPath spec in c5ceb2c.

@pyrmont pyrmont added needs-review The PR needs to be reviewed and removed author-action The PR has been reviewed but action by the author is needed labels Jul 1, 2019
@pyrmont pyrmont merged commit c5b7798 into rouge-ruby:master Jul 1, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jul 1, 2019

@MaximeKjaer Thanks for doing such a massive amount of work and for being so responsive to all our suggestions. Rouge is a better library as a result of this PR :)

@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Jul 1, 2019
@zyv
Copy link

zyv commented Jul 28, 2019

Awesome, thank you all! Hope GitHub updates to this version of Rouge soon.

@pyrmont
Copy link
Contributor

pyrmont commented Jul 28, 2019

@zyv My understanding is that GitHub has its own proprietary syntax highlighting library for the code displayed on GitHub proper. With GitHub Pages, GitHub does use Rouge but it's unfortunately a very old version. I'm hoping they update that soon :(

@JohannesLichtenberger
Copy link

Hope they update the version soon. I wrote the Support and in their Forum. Would be of utmost importance for me.

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.

5 participants