Add flyway variables support via placeholder templater #4026

srjonemed · 2022-11-03T22:56:55Z

Brief summary of the change made

Proposed solution for #4025

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

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 tox -e generate-fixture-yml).
- 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.

codecov · 2022-11-04T01:54:43Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (1c69cda) compared to base (29faf04).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #4026   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          190       190           
  Lines        14585     14585           
=========================================
  Hits         14585     14585

Impacted Files	Coverage Δ
src/sqlfluff/core/templaters/placeholder.py	`100.00% <ø> (ø)`
src/sqlfluff/core/config.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

barrywhart · 2022-11-04T11:39:28Z

src/sqlfluff/core/config.py

@@ -302,7 +302,7 @@ def _get_config_elems_from_file(cls, fpath: str) -> List[ConfigElemType]:
        # Disable interpolation so we can load macros
        kw: Dict = {}
        kw["interpolation"] = None
-        config = configparser.ConfigParser(**kw)
+        config = configparser.ConfigParser(delimiters="=", **kw)


Why was this added?

Per the configparser doc, "The first occurrence of a delimiting substring on a line is considered a delimiter" and it assumes both ':' and '=' to be delimiters by default. This would mean that the defining a key called "flyway:database = test_db_name" would split key="flyway" and val="database=test_db_name" :(
I see that all our keys use '=' as delimiter and ':' is only used in section names hence made this change. But, If there's a better way to not impose this globally, pls do let me know

It appears the current tests don't test for this change. See my comment on test/fixtures/templater/placeholder_flyway_var/placeholder_flyway_var_a.sql for a suggestion on how to test it.

You mentioned adding another test. Will any of your tests fail if this delimiters change is removed? I think it's important to have a test for this.

barrywhart · 2022-11-04T11:41:28Z

test/fixtures/templater/placeholder_flyway_var/placeholder_flyway_var_a.sql

@@ -0,0 +1,5 @@
+USE ${flyway:database}.test_schema;


I don't see any mention of flyway in the CI output. Perhaps you need to change something else to make this run? The placeholder templater tests are in this file: test/core/templaters/placeholder_test.py.

thx, will take a look

updated with a test, pls take a look

Curious, could u let me know if this fixture is needed - not sure where/how it'd be used

It's currently not used because the placeholder templater doesn't have a test that reads from file fixtures. See this jinja test: https://github.com/sqlfluff/sqlfluff/blob/main/test/core/templaters/jinja_test.py#L568L624

Can you look at adding a similar test for the placeholder templater? This would test the config delimiter change you made in a realistic way.

BTW, sorry it took me a while to respond. I had actually written the comment above several days ago, but I forgot to submit the pending comments. Occasionally GitHub confuses me. 😢

Thanks for the pointers, I'll have take a look soon

added a test to config_test.py that should hopefully cover the delimiters codecov

barrywhart

If the tests pass, this looks good to me. I'd perhaps like to take a look from my computer (currently on my phone) before merging.

srjonemed · 2022-11-04T20:12:12Z

Thanks!

srjonemed · 2022-11-09T06:34:13Z

@barrywhart i've pushed a new commit including adding another test. pls have a look..
Also, do we know when this pr could get merged and to what version?

barrywhart · 2022-11-10T22:54:06Z

See my comment about the need to add a config test for the = delimiter. If we can address that, the PR is ready for merge.

I think 1.4.2 will be released in the next few days.

barrywhart · 2022-11-04T20:57:05Z

src/sqlfluff/core/config.py

@@ -302,7 +302,7 @@ def _get_config_elems_from_file(cls, fpath: str) -> List[ConfigElemType]:
        # Disable interpolation so we can load macros
        kw: Dict = {}
        kw["interpolation"] = None
-        config = configparser.ConfigParser(**kw)
+        config = configparser.ConfigParser(delimiters="=", **kw)


It appears the current tests don't test for this change. See my comment on test/fixtures/templater/placeholder_flyway_var/placeholder_flyway_var_a.sql for a suggestion on how to test it.

barrywhart · 2022-11-04T21:01:45Z

test/fixtures/templater/placeholder_flyway_var/placeholder_flyway_var_a.sql

@@ -0,0 +1,5 @@
+USE ${flyway:database}.test_schema;


It's currently not used because the placeholder templater doesn't have a test that reads from file fixtures. See this jinja test: https://github.com/sqlfluff/sqlfluff/blob/main/test/core/templaters/jinja_test.py#L568L624

Can you look at adding a similar test for the placeholder templater? This would test the config delimiter change you made in a realistic way.

barrywhart · 2022-11-10T22:48:44Z

src/sqlfluff/core/config.py

@@ -302,7 +302,7 @@ def _get_config_elems_from_file(cls, fpath: str) -> List[ConfigElemType]:
        # Disable interpolation so we can load macros
        kw: Dict = {}
        kw["interpolation"] = None
-        config = configparser.ConfigParser(**kw)
+        config = configparser.ConfigParser(delimiters="=", **kw)


You mentioned adding another test. Will any of your tests fail if this delimiters change is removed? I think it's important to have a test for this.

barrywhart · 2022-11-10T22:54:56Z

test/core/templaters/placeholder_test.py

@@ -306,6 +322,8 @@ def test__templater_raw():
        "numeric_dollar_with_braces_and_string",
        "percent",
        "ampersand",
+        "flyway_var",
+        "flyway_var",


Why does flyway_var appear twice?

I saw some warning to the effect that ids list has less elements than the test sql's list
____________________________________________ ERROR collecting test/core/templaters/placeholder_test.py ____________________________________________ In test__templater_param_style: 17 parameter sets specified, with different number of ids: 16

I can remove and retest if above can be ignored

barrywhart

Requesting addition of a test for the = delimiter

srjonemed · 2022-11-13T08:17:49Z

Requesting addition of a test for the = delimiter

added test__config__load_placeholder_cfg to config_test.py, pls have a look if this satisfies the gap

barrywhart

Looks good! I temporarily removed the delimiters addition and verified that your new test fails. This will prevent future maintainers from simply removing that change and thinking it's safe because no tests fail.

Unfortunately, the new release happened earlier today, so this will have to wait for the next release (usually 2-4 weeks apart).

srjonemed · 2022-11-14T17:46:51Z

sounds good, thx!

srjonemed added 2 commits November 3, 2022 12:11

adding new style for supporting flyway placeholder variables in sql

5296ad7

add fixture for flyway style placeholder templating

8188f5b

barrywhart requested changes Nov 4, 2022

View reviewed changes

adding placeholder test for flyway_var style

03ca132

barrywhart approved these changes Nov 4, 2022

View reviewed changes

mandate flyway_var pattern be enclosed in curly brakcets always

1395d95

Merge branch 'main' into main

7e82da5

barrywhart reviewed Nov 10, 2022

View reviewed changes

barrywhart requested changes Nov 10, 2022

View reviewed changes

srjonemed and others added 3 commits November 12, 2022 22:45

adding test__config__load_placeholder_cfg

d200316

Merge branch 'main' of github.com:srjonemed/sqlfluff

b78278b

Merge branch 'main' into main

c7c6932

barrywhart approved these changes Nov 13, 2022

View reviewed changes

Merge branch 'main' into main

1c69cda

barrywhart merged commit 4d2a83c into sqlfluff:main Nov 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flyway variables support via placeholder templater #4026

Add flyway variables support via placeholder templater #4026

srjonemed commented Nov 3, 2022 •

edited by barrywhart

codecov bot commented Nov 4, 2022 •

edited

barrywhart Nov 4, 2022

srjonemed Nov 4, 2022

barrywhart Nov 4, 2022

barrywhart Nov 10, 2022

barrywhart Nov 4, 2022

srjonemed Nov 4, 2022

srjonemed Nov 4, 2022

srjonemed Nov 4, 2022

barrywhart Nov 4, 2022

barrywhart Nov 10, 2022

srjonemed Nov 11, 2022

srjonemed Nov 13, 2022

barrywhart left a comment

srjonemed commented Nov 4, 2022

srjonemed commented Nov 9, 2022

barrywhart commented Nov 10, 2022

barrywhart Nov 4, 2022

barrywhart Nov 4, 2022

barrywhart Nov 10, 2022

barrywhart Nov 10, 2022

srjonemed Nov 13, 2022 •

edited

barrywhart left a comment

srjonemed commented Nov 13, 2022

barrywhart left a comment

srjonemed commented Nov 14, 2022

Add flyway variables support via placeholder templater #4026

Add flyway variables support via placeholder templater #4026

Conversation

srjonemed commented Nov 3, 2022 • edited by barrywhart

Brief summary of the change made

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

Pull Request checklist

codecov bot commented Nov 4, 2022 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barrywhart left a comment

Choose a reason for hiding this comment

srjonemed commented Nov 4, 2022

srjonemed commented Nov 9, 2022

barrywhart commented Nov 10, 2022

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

srjonemed Nov 13, 2022 • edited

Choose a reason for hiding this comment

barrywhart left a comment

Choose a reason for hiding this comment

srjonemed commented Nov 13, 2022

barrywhart left a comment

Choose a reason for hiding this comment

srjonemed commented Nov 14, 2022

srjonemed commented Nov 3, 2022 •

edited by barrywhart

codecov bot commented Nov 4, 2022 •

edited

srjonemed Nov 13, 2022 •

edited