Support multiple controllers path #228
Conversation
… to add controllers_paths method
salmanasiddiqui
force-pushed
the
support_multiple_controllers_path
branch
from
0f13ad1 to
c420089
Compare
salmanasiddiqui
force-pushed
the
support_multiple_controllers_path
branch
from
c420089 to
0bb6edb
Compare
salmanasiddiqui
force-pushed
the
support_multiple_controllers_path
branch
from
0a54514 to
b122b8a
Compare
|
Hey @splittingred 👋🏽 , this PR is ready for review with the suggestion you made #213 (comment) |
| # Initialize the autoloaders with given controllers paths | ||
| # | ||
| # @param [String] controllers_path The path to Gruf Controllers | ||
| # @param [Array<String>] controllers_paths The path to Gruf Controllers | ||
| # | ||
| def load!(controllers_path:) | ||
| controllers(controllers_path: controllers_path) | ||
| def load!(controllers_path: nil, controllers_paths: []) | ||
| if controllers_path | ||
| ::Gruf.logger.warn('controllers_path is deprecated. Please use controllers_paths instead.') | ||
| controllers_paths = [controllers_path] | ||
| end | ||
| controllers(controllers_paths: controllers_paths) |
There was a problem hiding this comment.
Hey @splittingred 👋🏽
this is the change you requested in #213 (comment)
I have updated this accordingly. Let me know if you would like any other change
| # @param [Boolean] reloading | ||
| # @param [String] tag | ||
| # | ||
| def initialize(path:, reloading: nil, tag: nil) |
There was a problem hiding this comment.
This is a breaking change; can we avoid this by just checking to see if the passed variable is an Enumerable, or a String, and handle it accordingly? That would satisfy the requirement without breaking the public contract.
There was a problem hiding this comment.
i can leave the path keyword arg and additionally support paths. If both are passed then it would raise, otherwise path will be just wrapped in an array and set @paths
def initialize(path: nil, paths: [], reloading: nil, tag: nil)
raise ArgumentError(..) unless (path.nil? ^ paths.empty?)
@paths = paths.empty? ? [path] : pathsThere was a problem hiding this comment.
That's fine - we just need to make sure there's a backward-compatible deprecation path.
2 participants
What? Why?
I ran into a problem in a Rails app which was made up of multiple Rails engines. The problem was that two engines were using Gruf and when they were used in same Rails app, the Gruf config for
controllers_pathwas getting overridden.So to fix that problem, I patched Gruf to support multiple paths for controllers by adding a new config field
controllers_paths. This field defaults to[controllers_path]. Gruf uses thiscontrollers_pathsinstead ofcontrollers_patheverywhere, butcontrollers_pathis kept in config for backward compatibility.Note: I think the config should support something like
add_controllers_paths, instead of directly settingcontrollers_paths. That will allow multiple modules to add their controllers path without messing with the existing paths. Let me know if you like this idea, I will update the PRHow was it tested?
All the specs are passing