8000 fix request.service_key strip suffix by t0ch1k · Pull Request #225 · bigcommerce/gruf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix request.service_key strip suffix #225

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

t0ch1k
Copy link
@t0ch1k t0ch1k commented Feb 2, 2025

What? Why?

I don't want service_key to remove all the occurrences of .service, only to strip suffix

How was it tested?

within the existing unit tests + manually

@@ -87,7 +87,7 @@ def initialize(method_key:, service:, rpc_desc:, active_call:, message:)
# @return [String] The mapped service key
#
def service_key
@service.name.to_s.underscore.tr('/', '.').gsub('.service', '')
@service.name.to_s.underscore.tr('/', '.').delete_suffix('.service')
Copy link
Member

Choose a reason for hiding this comment

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

Hi @t0ch1k - can we add a unit test that covers your case explicitly? (that it deletes only the suffix, not all instances)

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow, sorry, I missed your message and only now saw it. Fixed :)

@t0ch1k t0ch1k force-pushed the fix-service-key branch from f10f00a to 5b7e5de Compare May 25, 2025 09:39
@t0ch1k t0ch1k force-pushed the fix-service-key branch from 5b7e5de to 9c39fae Compare May 25, 2025 09:41
@t0ch1k
Copy link
Author
t0ch1k commented May 29, 2025

unfortunately rubocop fails on the code I didn't touch :(

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
0