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

Move global configuration #403

Open
1 task done
sandstrom opened this issue Mar 20, 2024 · 18 comments
Open
1 task done

Move global configuration #403

sandstrom opened this issue Mar 20, 2024 · 18 comments
Assignees
Labels
enhancement New feature or request

Comments

@sandstrom
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe

If blueprinter is used for different things in a single codebase, the global configuration cannot be used.

For example, one might have two APIs (public and internal), and want to use blueprinter for both, but with different configuration for each.

Describe the feature you'd like to see implemented

Deprecate global configuration and move configuration to the base class.

Make it possible to override in subclasses, thereby relying on the common class inheritance pattern to handle "global configuration".

# One group of blueprinters
class PublicApiBlueprint < Blueprinter::Base
  # DSL alt 1
  config do
    sort_fields_by = :definition
  end

  # DSL alt 2
  config.sort_fields_by = :definition
end

class UserBlueprint < PublicApiBlueprint
  identifier :uuid

  fields :first_name, :last_name
end


# Another group of blueprinters
class InternalApiBlueprint < Blueprinter::Base
  # DSL alt 1
  config do
    sort_fields_by = :score
  end

  # DSL alt 2
  config.sort_fields_by = :score
end

class GroupBlueprint < InternalApiBlueprint
  identifier :id

  fields :name, :members_count
end

As for making sure configs are not shared (only copied) down the inheritance hiearchy, I'd probably pull in class_attribute from ActiveSupport (https://guides.rubyonrails.org/active_support_core_extensions.html#class-attributes).

If you want to add it manually there is some inspiration here:

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

@sandstrom Apologies for the delay here, and thanks for the detailed suggestion!

I think this would be a valuable addition to the library, and leveraging class_attribute for part of the implementation seems reasonable to me!

@lessthanjacob lessthanjacob added the enhancement New feature or request label Apr 7, 2024
@ritikesh
Copy link
Collaborator

class_attribute is a rails implementation and historically, we've shied away from doing rails specific implementations. We can however, use class instance variables.

@lessthanjacob
Copy link
Contributor

class_attribute is a rails implementation and historically, we've shied away from doing rails specific implementations. We can however, use class instance variables.

Not to split hairs here, but I wouldn't consider it explicitly a Rails implementation; this functionality was extracted from rails and is encapsulated entirely within an activesupport gem which can technically be used in any Ruby project.

If we don't want to bring in an intentional dependency on activesupport due to the side-effects it has on Ruby core classes, then I think that's a reasonable argument. The library is available to us currently due to our dependency on activerecord (which is partly why I wouldn't necessary be opposed to using it here), but truth be told I'd also like to fully remove that dependency as well.

@sandstrom
Copy link
Author

sandstrom commented Apr 10, 2024

I don't have a strong preference.

Should be fairly easy to replicate the logic of class_attribute from Rails, using class instance variables and the inherited method.

Breaking changes

What's your view on breaking changes and major releases? Some projects are happy about frequent major release, others prefer making them only once every year or two.

I'm fine with either, but it's good to know since this change could potentially be implemented as a breaking change, but also doable in a manner where global conf is used as fallback (non-breaking scenario).

@ritikesh
Copy link
Collaborator

This is a major refactor and I would personally prefer combining this with any/some other major deprecations planned for the near future as a potential 2.x release. Maybe this could go into a separate 2.x branch.

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

Still relevant!

@jhollinger
Copy link
Contributor

jhollinger commented Jun 12, 2024

I think I'm on board with this. "Global" config could go into an ApplicationBlueprint which all others could choose to inherit from or not.

I'd vote against using ActiveSupport for class_attribute. Pretty easy to accomplish the same thing using inherited.

@sandstrom
Copy link
Author

@jhollinger Do you know of a blog post or similar explaining how to replicate class_attribute behavior (more or less) with inherited?

Or are you simply thinking of something like this:

class Mother
  @things = []

  class << self
    attr_accessor :things
  end  

  def self.inherited(subclass)
    puts "New subclass: #{subclass}"
    subclass.things = self.things.dup
  end
end

class Child < Mother
end

@jhollinger
Copy link
Contributor

@sandstrom Yes, assuming we only have a handful of attributes to keep track of, the above is what I was thinking.

@RazvanFarte
Copy link

RazvanFarte commented Jun 27, 2024

Hi guys,

Hope I'm on the right topic. Should this mean that after this feature is implemented, it will be possible to have two separate sort_fields_by configs, per class level?

I'm trying to work around an issue with setting this configs, that impacts the views and associations in another Blueprint.
The issue happens due to this line sorted_fields = sort_by_definition ? sort_by_def(view_name, fields) : fields.values.sort_by(&:name) in fields_for.
And the only fix found was to have the configure block before and after the first Blueprint definition, which looks ugly to me. Would prefer sandstorm's version. Not sure if it rings a bell, but I could open a new issue, so it could be referenced here and hopefully some configs to be available at Blueprint level, instead of global.

Here the fix I found that works for me:

Blueprinter.configure do |config|
  config.sort_fields_by = :definition
end

module CustomerApi
  class BaseBlueprint < Blueprinter::Base
  end
end

Blueprinter.configure do |config|
  config.sort_fields_by = :name_asc
end

@sandstrom
Copy link
Author

Should this mean that after this feature is implemented…

Yes, that would be possible

Copy link

github-actions bot commented Sep 3, 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

Not stale

@jhollinger
Copy link
Contributor

FYI, this is now part of a proposed V2 base class in #437 (along with other changes). All configuration, and fields, are held in Blueprint classes and are inheritable. To simulate "global" config in V2, options and default fields can be configured in an ApplicationBlueprint class which others inherit from.

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
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants