procore-oss / blueprinter

Simple, Fast, and Declarative Serialization Library for Ruby
MIT License
1.11k stars 108 forks source link

Separate internal options from external ones #405

Open sandstrom opened 5 months ago

sandstrom commented 5 months ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe

Right now options are used both internally and externally. For example, :block and :association` are blessed options with internal purpose.

They are sometimes 'scrubbed out' for example this line in AssociationExtractor:

options_without_default = options.except(:default, :default_if)

This increases the risk of colliding with user-defined options, and also make the internal code slightly harder to follow/modify.

Describe the feature you'd like to see implemented

I would separate the two sets, in one of two ways:

  1. Either into e.g. internal_options and options, and keep things such as :block in the internal set, or
  2. turn internal options into proper instance variables, such as @field_block (see example below).

I'd prefer the second way, but (1) would at least be an improvement over the current state.

# base.rb
def self.field(method, options = {}, &block)
  current_view << Field.new(
    method,
    options.fetch(:name) { method },
    options.fetch(:extractor) { Blueprinter.configuration.extractor_default.new },
    self,
    inline_definition: block)
  )
end

# field.rb
class Field
  attr_reader :method, :name, :extractor, :options, :blueprint, :inline_definition

  def initialize(method, name, extractor, blueprint, inline_definition: nil, options = {})
    @method = method
    @name = name
    @extractor = extractor
    @blueprint = blueprint
    @options = options
    @inline_definition = inline_definition
  end

Describe alternatives you've considered

No response

Additional context

No response

sandstrom commented 5 months ago

Happy to discuss this in more detail, and elaborate more.

lessthanjacob commented 5 months ago

I'd definitely like to see a bit more intentionality here.options feels like this sort of "grab bag" of information, and I certainly agree that it increases the cognitive overhead when reading through it.

So in that sense, I'd also push more towards your second proposed option.

github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sandstrom commented 2 months ago

Still relevant!

github-actions[bot] commented 4 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sandstrom commented 3 weeks ago

Not stale