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

port deep parameter logging from Badiapp/grape_logging #37

Merged
merged 3 commits into from
Mar 6, 2017

Conversation

jason-uh
Copy link
Contributor

@jason-uh jason-uh commented Jan 2, 2017

This ports deep parameter logging from Badiapp/grape_logging without having to depend upon Rails directly (lifts the ParameterFilter from Rails, or uses it indirectly if Rails is available). I also started to add some specs.

@jason-uh
Copy link
Contributor Author

jason-uh commented Mar 1, 2017

@aserafin Is there any chance of getting this PR merged and a new gem built?

def safe_parameters(request)
# Now this logger can work also over Rails requests
return clean_parameters(request.env[AD_PARAMS] || {}) if request.params.empty?
clean_parameters(request.params.clone)
Copy link
Owner

Choose a reason for hiding this comment

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

looking at ParameterFilter class I don't think we need to clone parameters anymore, right?

Copy link
Owner

Choose a reason for hiding this comment

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

could we also do if ... else ... end here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☑️ Done and done.

@aserafin
Copy link
Owner

aserafin commented Mar 1, 2017

hey @jason-uh - sorry it took so long and thanks for contribution, especially the tests 💪 could you take a look at two notes I left?

@aserafin aserafin merged commit cc35a9a into aserafin:master Mar 6, 2017
@jason-uh
Copy link
Contributor Author

jason-uh commented Mar 7, 2017

Thanks @aserafin.

@jason-uh
Copy link
Contributor Author

jason-uh commented Mar 7, 2017

@aserafin would you also publish a new version of the gem? Thanks.

@aserafin
Copy link
Owner

@jason-uh
Copy link
Contributor Author

Awesome @aserafin. Thanks a bunch, and thanks to @oricodes89 from @Badiapp for the initial code.

@oricodes89
Copy link

Thanks to @dsantosmerino! @Badiapp is happy to help.

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.

4 participants