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

UrlQueryHelper parameter removals #851

Merged
merged 6 commits into from Jan 4, 2017

Conversation

Projects
None yet
2 participants
@demiankatz
Copy link
Member

commented Nov 9, 2016

Continuing work from #850, this removes the $escape and $paramArray parameters that add unnecessary noise to the interface and are now better addressed through the fluent interface. Unfortunately, this does significantly impact the parameter lists for the removeFacet() and updateQueryString() methods, since the $escape parameter fell in the middle of those lists.

TODO

  • Discuss whether these changes (most significantly, the removeFacet() parameter list change, since that is a public method) are acceptable and/or whether we need to mitigate them somehow.
  • Test

demiankatz added some commits Nov 9, 2016

Eliminate unnecessary $escape parameters.
- Significantly changes signatures of removeFacet() and updateQueryString()
@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Nov 9, 2016

@EreMaijala, as promised, further cleanup work. I'm interested in your thoughts when you have a chance.

@demiankatz demiankatz requested a review from EreMaijala Dec 22, 2016

@EreMaijala
Copy link
Contributor

left a comment

I think this is a good change, and it's fairly easy to find and fix the code that call the changed methods.

@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2016

Thanks, @EreMaijala ! Looks like there are some failing tests I hadn't noticed before, though. I'll investigate next week.

@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2017

I fixed the bug that was causing test failures and ran the full integration suite. I think we're ready to go with this now!

@demiankatz demiankatz merged commit 859cbbb into master Jan 4, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@demiankatz demiankatz deleted the urlqueryhelper-deprecations branch Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.