Open rusikf opened 2 months ago
I agree. I also want to point out that the meta programming done in this library has multiple issues, including the one mentioned:
Dynamically loads files on method calls, as the issue author mentioned. Loading the codegen library could be restructured so that it "just works" using zeitwerk, this is the defacto ruby community standard for code loading and is highly efficient and configurable, as long as the library sets up standard conventions to use.
Dynamically requires and loads class names based off of hash parameter names. So if the wrong parameter name is used (which is easy to do due to the lack of documentation), the caller gets another cryptic load error. Example:
A big time lack of documentation, would be nice to be able to figure out how to use this library without navigating the source code of this project (which isn't the easiest to do due to all of the dynamic code generation and delegation). Tools like YARD allow you to write your own DSL to generate documentation. With some conventions and a better standard interface for defining the code generation, it seems fairly straight forward to generate this.
Dynamically regenerates methods on class instance instantiation, even if the method was already defined previously due to prior instantiation. This is due to these methods being defined inside of the initialization call. I'm not sure why these methods need to be redefined with the same interface every time the wrapping container class is initialized. One solution is to restructure this library to use a class method to define the classes and modules:
module Hubspot
module Discovery
module BaseModuleClient
attr_reader :params
# Defining `initialize` in a module to be included also feels like a
# code smell, at this point why not just use a base class to
# inherit from?
def initialize(params)
@params = params
end
class << self
def included(base)
base.extend(ClassMethods)
end
end
module ClassMethods
def api_modules(*module_names)
module_names.each do |module_name|
# Call code that create methods here
end
end
def api_classes(*class_names)
class_names.each do |module_name|
# Call code that create methods here
end
end
end
end
end
end
Then instead of something like:
def api_classes
%i[
pipeline_stages
pipeline_stage_audits
pipelines
pipeline_audits
].freeze
end
It becomes:
api_classes :pipeline_stages,
:pipeline_stage_audits,
:pipelines,
:pipeline_audits
I'm sure other solutions also exist.
Hi Hubspot Team!
On calling most API methods with invalid params We don't have validation because of dynamic requiring files Idea :bulb: - Can we remove the dynamic code required?
The ability to see the raw backtrace/logs/validation errors is much better than LoadError
Note: Everything works if pass valid parameters.
Thanks!