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

Ignores args #34

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Ignores args #34

merged 3 commits into from
Jan 22, 2025

Conversation

pizcadesaber
Copy link
Contributor

Ignore args and change $method to $target based on 'wire:target' for shared rate limiters.

@danharrin
Copy link
Owner

Please explain "ignore args" and the changes in this PR

@pizcadesaber
Copy link
Contributor Author

pizcadesaber commented Jan 22, 2025

Rate limits only need the function name, so I used an option to exclude function arguments from the backtrace. This improves performance.

@danharrin
Copy link
Owner

Please rename $method back to $target as it is a breaking change. Please add the limit: named arguments back.

@pizcadesaber
Copy link
Contributor Author

I have already renamed the parameters to $method.

But are there intentions to change them? It might be clearer to call it something like target, as it could be used in situations where you want to apply the same rate limit to multiple methods.

@pizcadesaber pizcadesaber changed the title Ignores args and changes $method to $target Ignores args Jan 22, 2025
@danharrin
Copy link
Owner

I don't want to change it at the moment since it is a breaking change.

Please put back the named arguments that you removed

@pizcadesaber
Copy link
Contributor Author

pizcadesaber commented Jan 22, 2025

I believe they are already renamed as I mentioned in the second commit. Should I do anything else?

Edit: Now I just realized what you mean. I’ll do it, but I think it’s unnecessary in this case.

@danharrin
Copy link
Owner

It is a styling choice, it makes it clearer in the code what 2 is.

@danharrin danharrin merged commit 18680be into danharrin:main Jan 22, 2025
6 checks passed
@danharrin
Copy link
Owner

Thanks

@pizcadesaber pizcadesaber deleted the ignore-args branch January 22, 2025 20:29
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.

2 participants