blueprinter icon indicating copy to clipboard operation
blueprinter copied to clipboard

Implement association as it's own thing

Open sandstrom opened this issue 1 year ago • 17 comments
trafficstars

Is there an existing issue for this?

  • [X] I have searched the existing issues

Is your feature request related to a problem? Please describe

Right now, association is internally implemented as a field, with a magic :assocation option.

Describe the feature you'd like to see implemented

I'd implement association as a proper class internally, similar to Field.

    def self.association(method, options = {}, &block)
      validate_blueprint!(options[:blueprint], method)

      make_association(
        method,
        options.merge(
          extractor: options.fetch(:extractor) { AssociationExtractor.new }
        ),
        &block
      )
    end

Describe alternatives you've considered

No response

Additional context

No response

sandstrom avatar Mar 21 '24 12:03 sandstrom

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

sandstrom avatar Mar 21 '24 12:03 sandstrom

Friendly ping @lessthanjacob @njbbaer @ritikesh

If you think this is a reasonable suggestion, let me know! If so, I'd propose these steps:

  • I'll flesh out the proposed change in some more detail (written, in this issue)
  • You get another chance to provide more feedback, and if you're happy we'll open a PR

For reference, there are also a few other issues that I've opened, where I'd also be happy to get your input.

Full list

  • https://github.com/procore-oss/blueprinter/issues/403
  • https://github.com/procore-oss/blueprinter/issues/404
  • https://github.com/procore-oss/blueprinter/issues/405
  • https://github.com/procore-oss/blueprinter/issues/406
  • https://github.com/procore-oss/blueprinter/issues/407

sandstrom avatar Apr 04 '24 07:04 sandstrom

Friendly ping @lessthanjacob @njbbaer @ritikesh

Happy to hear your thoughts on this!

sandstrom avatar Apr 11 '24 09:04 sandstrom

What would the responsibilities be of this Association class outside of how it's being handled within the ViewCollection/View contexts today?

ritikesh avatar Apr 11 '24 12:04 ritikesh

It would be a sibling class next to Field. It would work just the same as the Field class does today.

Mostly just seemed like a design that's easier to comprehend, because right now an association is a field with the "magic" option called :association with value true.

That said, I don't feel strongly about this.

An alternative would be to turn it into a property on the field (similar to what's proposed for the other magic options in https://github.com/procore-oss/blueprinter/issues/405).

sandstrom avatar Apr 11 '24 13:04 sandstrom

They do behave similarly. It's just a readability thing with no major functional differences barring associations having their own blueprinter/view based rendering.

An alternative would be to turn it into a property on the field (similar to what's proposed for the other magic options in https://github.com/procore-oss/blueprinter/issues/405).

At this point, I'm not strongly for/against this one either. But the trend seems to be in the direction of major rewrites and as long as there's no/little impact to the consumers, this should still be ok.

@jmeridth / @njbbaer - any thoughts from your side?

ritikesh avatar Apr 11 '24 18:04 ritikesh

in fact, many fields have additional options too - like date_time formatter (which we're planning to get rid off too in favour of extractors). If we're able to achieve consistency in terms of the options that required for defining a field and an association with them being unique to their own types, then I think this might make more sense.

ritikesh avatar Apr 11 '24 18:04 ritikesh

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.

github-actions[bot] avatar Jun 30 '24 01:06 github-actions[bot]

Not stale

sandstrom avatar Jul 04 '24 19:07 sandstrom

I didn't link this issue, but I recently started down this path with #414, albeit in a very rudimentary way for the time being.

https://github.com/procore-oss/blueprinter/blob/2150b9282e5352ba9a01ee947fd8f1e700171a4a/lib/blueprinter/association.rb#L1-L33

This makes the interface a bit more explicit/clearer, but there's room to take this further, since it's mostly functioning as a facade around Field.new anyway.

lessthanjacob avatar Jul 05 '24 19:07 lessthanjacob

This makes the interface a bit more explicit/clearer, but there's room to take this further, since it's mostly functioning as a facade around Field.new anyway.

That's great!

sandstrom avatar Jul 06 '24 13:07 sandstrom

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.

github-actions[bot] avatar Sep 05 '24 01:09 github-actions[bot]

Not stale

sandstrom avatar Sep 05 '24 07:09 sandstrom

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.

github-actions[bot] avatar Nov 17 '24 02:11 github-actions[bot]

Not stale

sandstrom avatar Nov 18 '24 09:11 sandstrom

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.

github-actions[bot] avatar Jan 19 '25 02:01 github-actions[bot]

not stale

sandstrom avatar Jan 19 '25 18:01 sandstrom