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

update regex_comments string in check-script-has-no-table-name #47

Merged
merged 2 commits into from
Jan 27, 2022
Merged

update regex_comments string in check-script-has-no-table-name #47

merged 2 commits into from
Jan 27, 2022

Conversation

neddonaldson
Copy link
Contributor

fixes #46

@neddonaldson neddonaldson changed the title update regex_comments string update regex_comments string in check-script-has-no-table-name Dec 18, 2021
@neddonaldson
Copy link
Contributor Author

hey @tomsej wanted to call your attention to this, which solves what may also be a blocker for some other folks trying to upgrade to v1.0.0

@tomsej
Copy link
Contributor

tomsej commented Jan 24, 2022

Sorry @neddonaldson . I was solving some family issues. Will look into this tomorrow!

@neddonaldson
Copy link
Contributor Author

@tomsej no need to apologize! Appreciate you taking a look

@tomsej
Copy link
Contributor

tomsej commented Jan 26, 2022

Hi @neddonaldson, thank you for this PR! It is tricky! But your regex is much better than mine. Unfortunately test_replace_comments is failing on

sql = "/* select * from ee*/"
assert replace_comments(sql) == ""

It is returning "/**/". Think fixing the test is enough since replace_comment is used only for ignoring table names in comments so it does not matter if it is returning blank comments. Can you please fix the falling test? Thank you!

@neddonaldson
Copy link
Contributor Author

@tomsej ready for your eyes again. Just added an option for the test to pass with "/**/" per your note that "it does not matter if it is returning blank comments". Passed locally but it doesn't look like I can run workflows to check in github actions
Screen Shot 2022-01-26 at 10 22 56 AM

@codecov-commenter
Copy link

Codecov Report

Merging #47 (588b9a5) into main (4100314) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #47   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           45        45           
  Lines         1956      1956           
  Branches       256       258    +2     
=========================================
  Hits          1956      1956           
Impacted Files Coverage Δ
pre_commit_dbt/check_script_has_no_table_name.py 100.00% <ø> (ø)

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 4100314...588b9a5. Read the comment docs.

@tomsej
Copy link
Contributor

tomsej commented Jan 27, 2022

Thank you @neddonaldson looks good! Hope new version will be released soon

@tomsej tomsej merged commit cc144b9 into dbt-checkpoint:main Jan 27, 2022
@neddonaldson neddonaldson deleted the patch-1 branch January 27, 2022 06:02
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.

Regex Parsing Leads to Hanging Test
3 participants