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

Query string all #338

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Query string all #338

merged 5 commits into from
Aug 6, 2024

Conversation

dgitis
Copy link
Collaborator

@dgitis dgitis commented Aug 6, 2024

Description & motivation

Initially, I wanted to add an *all* flag to the remove query string feature.

When writing the tests, I discovered that the remove query string only removed question and ampersand query strings but not hash strings which may not technically be query strings but are sometimes used as such.

In fixing the regular expression, I ended up simplifying it and added tests to ensure that the new regex works as expected.

We may want to add the following to the release notes when pushing this live.

The query_parameter_exclusions variable now removes fragments, hash # parameters, from page_location and page_referrer. In most cases this will be an improvement in functionality. However, if you have query parameter keys using question mark, ?, or ampersand, &, that you want to remove and fragments that you don't want to remove, then you will need to override the remove_query_parameters macro.

Checklist

  • [y ] I have verified that these changes work locally
  • [y ] I have updated the README.md (if applicable)
  • [y ] I have added tests & descriptions to my models (and macros if applicable)
  • [y ] I have run dbt test and python -m pytest . to validate existing tests

@adamribaudo-velir
Copy link
Collaborator

Looks great. thanks for adding the unit test. I need to figure out how to get the dbt unit tests to run on PR updates, but I can do that separately. The unit tests passed.

@adamribaudo-velir adamribaudo-velir merged commit 8962a25 into main Aug 6, 2024
2 checks passed
@adamribaudo-velir adamribaudo-velir deleted the query-string-all branch August 6, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants