Skip to content
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

Separate internal options from external ones #405

Open
1 task done
sandstrom opened this issue Mar 21, 2024 · 8 comments
Open
1 task done

Separate internal options from external ones #405

sandstrom opened this issue Mar 21, 2024 · 8 comments
Assignees

Comments

@sandstrom
Copy link

sandstrom commented Mar 21, 2024

Is there an existing issue for this?

  • I have searched the existing issues

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 sandstrom mentioned this issue Mar 21, 2024
1 task
@sandstrom
Copy link
Author

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

@lessthanjacob
Copy link
Contributor

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.

Copy link

github-actions bot commented Jun 7, 2024

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
Copy link
Author

Still relevant!

Copy link

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
Copy link
Author

Not stale

Copy link

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
Copy link
Author

Not stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants