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

Short-circuit nested-params when already applied or empty #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vizanto
Copy link

@vizanto vizanto commented Nov 18, 2015

Instead of just complaining, I'd figure I make a pr :-)

@weavejester
Copy link
Member

Thanks for the patch. However, I'm uncertain of the use case. Under what circumstances would this be useful? That is, under what scenarios could the nested-params middleware legitimately be applied more than once?

Second, you use metadata, but you attach the metadata before compilation to the following form and not, as I suspect you intended, to the returned parameter map. You'd need to use the with-meta function for your patch to work.

Third, you have no tests for this change. Tests protect against regressions and allow me to see your patch is working. It also helps you see your own patch is working!

Fourth, the "more precise doc-strings" commit isn't directly related to this PR, so it should be in a separate PR.

Fifth, you assume that the metadata for the parameters remains, but operations on data structures will frequently clear metadata. This makes it an unreliable mechanism for checking if the request map has had the nested parameters middleware applied.

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