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

Simplify rule creation by adding a functional API to RuleContext #2126

Conversation

barrywhart
Copy link
Member

@barrywhart barrywhart commented Dec 16, 2021

Brief summary of the change made

Makes progress on #1808

Are there any other side effects of this change that we should be aware of?

  • Updates many existing rules to use the new classes (see changes to rules.rst for the current list).
  • Updates L041 to use awesome-pattern-matching package

These changes generally make the rule code:

  • Shorter
  • Less complex
  • More declarative & functional

This style of code should be easier for heavy SQL users and mathematicians to author. 👍🏽💪🏽

Reviewers may also wish to look at this branch, which implements a new rule (suggested in #1799) using the new API. I thought it'd be a good test case to create a new rule, not just rewrite existing ones. Note that it's a potentially pretty tricky rule, but it lacks some of the typical "code smells" of past tricky rules, e.g. complex loops, lots of if statements, heavy use of segment indices.

Pull Request checklist

  • Please confirm you have completed any of the necessary steps below.

  • Included test cases to demonstrate any code changes, which may be one or more of the following:

    • .yml rule test cases in test/fixtures/rules/std_rule_cases.
    • .sql/.yml parser test cases in test/fixtures/dialects (note YML files can be auto generated with python test/generate_parse_fixture_yml.py or by running tox locally).
    • Full autofix test cases in test/fixtures/linter/autofix.
    • Other.
  • Added appropriate documentation for the change.

  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

@barrywhart barrywhart marked this pull request as draft December 16, 2021 17:13
@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #2126 (3880142) into main (f33be99) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              main     #2126    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          150       155     +5     
  Lines        10886     10988   +102     
==========================================
+ Hits         10886     10988   +102     
Impacted Files Coverage Δ
src/sqlfluff/core/parser/segments/base.py 100.00% <100.00%> (ø)
src/sqlfluff/core/rules/base.py 100.00% <100.00%> (ø)
src/sqlfluff/core/rules/functional/__init__.py 100.00% <100.00%> (ø)
...core/rules/functional/raw_file_slice_predicates.py 100.00% <100.00%> (ø)
.../sqlfluff/core/rules/functional/raw_file_slices.py 100.00% <100.00%> (ø)
...lfluff/core/rules/functional/segment_predicates.py 100.00% <100.00%> (ø)
src/sqlfluff/core/rules/functional/segments.py 100.00% <100.00%> (ø)
src/sqlfluff/rules/L001.py 100.00% <100.00%> (ø)
src/sqlfluff/rules/L004.py 100.00% <100.00%> (ø)
src/sqlfluff/rules/L008.py 100.00% <100.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33be99...3880142. Read the comment docs.

@barrywhart
Copy link
Member Author

@jpy-git, @tunetheweb: Please take a preliminary look at this. I'm not looking for a typical "pre-merge" review. This is more of a design review:

  • Does this look like a good start towards making it easier to write rules?
  • Do you see any areas where the API might be improved?
  • Are there other rules we should look at updating to use this new API, as a kind of "beta test"? I can update them, or I'd be happy for you to update them instead.

Note that some of the functions on the new classes are just stubs (they raise NotImplementedError). If/when we get to a more proper review, I can implement these functions or remove them for now (i.e. add them later when needed).

Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

@barrywhart Like it a lot! 😎 Added my initial thoughts/questions. See if there's anything you think we should change off the back of that and then I can maybe try applying this to L009 to get a better idea of the user experience 👍

src/sqlfluff/core/rules/base.py Show resolved Hide resolved
src/sqlfluff/core/rules/base.py Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/surrogates/__init__.py Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/surrogates/segments.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L050.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L050.py Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/surrogates/segments.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

A couple more random thoughts to add, shaping up nicely!

Will have a more in depth look at the edited rules tomorrow 😄

src/sqlfluff/core/rules/surrogates/segments.py Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/surrogates/segments.py Outdated Show resolved Hide resolved
src/sqlfluff/core/rules/surrogates/segments.py Outdated Show resolved Hide resolved
src/sqlfluff/rules/L013.py Outdated Show resolved Hide resolved
Copy link
Member

@alanmcruickshank alanmcruickshank left a comment

Choose a reason for hiding this comment

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

Code looks great - docs aren't building properly at the moment but otherwise love it 👍

Comment on lines 49 to 65
* `L001`
* `L008`
* `L009`
* `L013`
* `L015`
* `L017`
* `L021`
* `L025`
* `L031`
* `L035`
* `L036`
* `L038`
* `L042`
* `L043`
* `L047`
* `L050`
* `L052`
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it makes sense to keep this list given how many you've converted. It will just end up out of date. If we want to still give some measure of scale, but without having to maintain the list, it might be better to say something like: ...at 2021-12-30, 17 rules use this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

Comment on lines 67 to 77
.. autoclass:: sqlfluff.core.rules.functional.segments
:members:

.. autoclass:: sqlfluff.core.rules.functional.segment_predicates
:members:

The `_eval` function of each rule should take enough arguments that it can
evaluate the position of the given segment in relation to its neighbors,
and that the segment which finally "triggers" the error, should be the one
that would be corrected OR if the rule relates to something that is missing,
then it should flag on the segment FOLLOWING, the place that the desired
element is missing.
.. autoclass:: sqlfluff.core.rules.functional.raw_file_slices
:members:

.. autoclass:: sqlfluff.core.rules.functional.raw_file_slice_predicates
:members:
Copy link
Member

Choose a reason for hiding this comment

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

These autodoc commands don't seem to render properly at the moment:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes incoming...

Copy link
Member

Choose a reason for hiding this comment

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

@barrywhart - can you push your most recent changes? I can't see them on the branch. I'll give them a spin when they're up.

@barrywhart
Copy link
Member Author

@alanmcruickshank: Thanks for checking the docs! I hadn't built and previewed those yet. Should be good now.

@barrywhart
Copy link
Member Author

@alanmcruickshank: Okay, the fixes are up now. (Earlier, I had accidentally pushed them to the wrong branch.)

@jpy-git
Copy link
Contributor

jpy-git commented Jan 1, 2022

@barrywhart I'm conscious that things are still getting added to this PR and we still haven't reviewed the awesome-pattern-matching dependency.

Looking at the repo it seems like quite a small project?
image
Only 700 downloads in the past month: https://pypistats.org/packages/awesome-pattern-matching

This issue here mentions there may be a lot of overlap with the matching introduced in Python 3.10
(relevant PEP: https://www.python.org/dev/peps/pep-0622/)
So maybe there's value in seeing if there's an available backport for that instead?

I feel like maybe it's best to introduce the functional API and it's rule changes/documentation in this PR, as there are no concerns there and it's been pretty thoroughly reviewed, and separate the pattern matching stuff into it's own PR afterwards. They're not really related, other than both touching rules, and would be good to have any discussion on apm separated for future reference. I can see that the functional API has been linked to several issues so we should get that merged to unblock them.

@barrywhart
Copy link
Member Author

@jpy-git: That's definitely valid feedback. I'm happy to remove the pattern matching stuff for now.

I did take a look at the new built-in pattern matching in Python 3.10, and there is a backport (although the backport looks new and kind of unofficial). From my evaluation, the built-in pattern matching is less powerful, e.g. it doesn't support patterns with both leading and trailing "don't care" sections. It's also unclear if it supports matching across multiple levels of a tree structure.

OTOH:

TL;DR I'll remove it from this PR. I may keep tinkering with it, possibly contributing to it. Among the packages out there, it comes closest to my dream of "regex-like power applied to arbitrary objects, plus support for patterns that span levels of a tree". (AFAICT, most of them aren't even trying to do one or both of these.) Part of the appeal is that the patterns closely resemble SQLFluff dialect definitions, so could be very approachable and intuitive for contributors.

@jpy-git
Copy link
Contributor

jpy-git commented Jan 1, 2022

Makes sense, I def like the concept so it's worth investigating further afterwards 😄

@barrywhart barrywhart changed the title Simplify rule creation: 1) Add a functional API to RuleContext 2) L041: Use a pattern-matching package to matching segment sequences Simplify rule creation by adding a functional API to RuleContext Jan 1, 2022
@barrywhart
Copy link
Member Author

@jpy-git: Ok, I've removed all references to and usage of awesome-pattern-matching. Ready for a closer review?

I created an issue, #2221, to revisit the package in the future, and for reference, that issue also includes the rewritten rules and documentation that was removed from this PR.

Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

small mypy cleanup otherwise good 👍

mypy.ini Outdated Show resolved Hide resolved
Copy link
Contributor

@jpy-git jpy-git left a comment

Choose a reason for hiding this comment

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

Looks good! 👍 Think we can get this merged if no one else has any objections? 🚀

@jpy-git jpy-git dismissed alanmcruickshank’s stale review January 2, 2022 15:19

Docs issue has been fixed :)

@barrywhart barrywhart merged commit 043011f into sqlfluff:main Jan 2, 2022
@barrywhart
Copy link
Member Author

I went ahead and merged. I hope that was okay? 😬 @alanmcruickshank

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.

3 participants