search

elixir-grpc / grpc-reflection

elixir graph reflection support
Apache License 2.0
9 stars 6 forks source link

Use Macro.camelize/1 #36

Closed christopherlai closed 2 months ago

christopherlai commented 3 months ago

Fixes a bug with package names that contain underscores like package test_service.v1.FooBar. This will generate a module named TestService.V1.FooBar in Elixir.

Currently if a package with underscore is used, it results in the following list ["Test_service", "V1", "FooBar"] and fails Module.safe_concat/1 because this atom does not exist. Test_service should be TestService.

christopherlai commented 3 months ago

I would have preferred an issue for better conversation, but 🤷

You have removed the test for upcase_first/1 and the only place that it is called, but you didn't remove the now unused and untested def upcase_first(...). Please also remove the function definition when it is unused and untested.

Otherwise this looks good

Sorry about that. Seemed like a simple issue so I opened a PR. Will remember to start a discussion in the future.

Removed upcase_first.

mjheilmann commented 2 months ago

I would have preferred an issue for better conversation, but 🤷 You have removed the test for upcase_first/1 and the only place that it is called, but you didn't remove the now unused and untested def upcase_first(...). Please also remove the function definition when it is unused and untested. Otherwise this looks good

Sorry about that. Seemed like a simple issue so I opened a PR. Will remember to start a discussion in the future.

Removed upcase_first.

No worries really, I'm just a cross the Ts and dot the Is kind of guy.

mjheilmann commented 2 months ago

Thanks for the work @christopherlai !

christopherlai commented 2 months ago

Thanks for the work @christopherlai !

Happy to help, but it was purely self-serving. Needed this fix for myself.

What's the schedule for cutting a new release hex?

mjheilmann commented 2 months ago

I'll see about a patch release with this and another bugfix (already merged) probably tomorrow

mjheilmann commented 2 months ago

patch is out as 0.1.4