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

Refactor regex part of macros #56

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Refactor regex part of macros #56

merged 2 commits into from
Dec 21, 2021

Conversation

fridgepoet
Copy link
Member

@fridgepoet fridgepoet commented Dec 14, 2021

This will extract the matching of macro name and the extraction of fields in rawSQL in a separate function to facilitate testing.

Copy link
Collaborator

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks @fridgepoet! I have some suggestions. BTW, can you add some description in the PR? This is useful for other people to understand why you are making the change (and to leave it in the history).

macros_test.go Outdated Show resolved Hide resolved
macros_test.go Outdated
assert.Equal(t, `\$__some_string\b(?:\((.*?)\))?`, getMacroRegex("some_string"))
}

func TestGetMatches_does_not_match_for_macro_name_which_is_substring(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

to have them all under the same test, you can move this to another t.Run within TestGetMacroRegex

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

My mistake. It's now separated into the test for getMacroRegex and getMatches

Copy link
Collaborator

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@fridgepoet fridgepoet merged commit 14ea102 into main Dec 21, 2021
@fridgepoet fridgepoet deleted the refactor-regex branch December 21, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants