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

Global if/unless Procs for Conditional Field Rendering #127

Merged
merged 13 commits into from
Jan 18, 2019

Conversation

mcclayton
Copy link
Contributor

@mcclayton mcclayton commented Jan 15, 2019

Currently, you can conditionally render fields by setting a field-level options argument :if or :unless.
This PR enables the setting of a global :if and :unless proc that will be used to evaluate the conditional render of all fields.
The field-level setting overrides the global config setting (for the field) if both are set.

Field-level Setting

class UserBlueprint < Blueprinter::Base
  identifier :uuid
  field :last_name, if: ->(user, _field_name, options) { user.first_name != options[:first_name] }
  field :age, unless: ->(user, _field_name, _options) { user.age < 18 }
end

Global Config Setting

Blueprinter.configure do |config|
  config.if = ->(obj, field_name, _options) { !obj[field_name].nil? }
  config.unless = ->(obj, field_name, _options) { obj[field_name].nil? }
end

This PR also addresses making #124 easier/cleaner to accomplish

Copy link
Contributor

@philipqnguyen philipqnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a major breaking change.

What's the need for passing in a field_name? I'm confused.

@mcclayton
Copy link
Contributor Author

@philipqnguyen Yes, I called this out as breaking in the CHANGELOG, The need is that, since the global :if and :unless procs are called at field-evaluation time, there's no way to see what field is being evaluated in the proc. So there'd be no way to accomplish #124 without passing in the field_name to do:

Blueprinter.configure do |config|
  config.if = ->(obj, field_name, _options) { !obj[field_name].nil? }
  config.unless = ->(obj, field_name, _options) { obj[field_name].nil? }
end

Make sense? Or am I missing a way to still be able to do this without passing in the field_name?

@philipqnguyen
Copy link
Contributor

philipqnguyen commented Jan 17, 2019

2 things off the top of my head:

  1. I'm wondering if we can merge field_name into options and not be breaking?
  2. If we decide to do a breaking change, :if is a pretty commonly used option, and to change it's behavior so much, I think we should consider a major version bump to 1.0.0. However, moving to 1.0.0 will place a big responsibility on us because then we MUST adhere to semantic versioning.

While we have snuck in breaking changes before in 0.2.0 and 0.3.0 because technically versions that are pre-1.0.0 makes no guarantee on breaking changes. However, Blueprinter hasn't had a breaking change for a long time, and it's been very stable between 0.3.0 through 0.10.0, therefore I'm worried about surprising people with a breaking change all of a sudden.

@mcclayton
Copy link
Contributor Author

Yeah we can totally merge the field_name into the option if you think that's kosher. I think that's a little unexpected to a user that we'd add it to the options as options are something that the user should be defining not that we'd ad-hoc add to. I definitely get your desire to avoid this sort of breaking change though. Global if/unless procs are not fully useful without the field_name though 🤔

@philipqnguyen
Copy link
Contributor

philipqnguyen commented Jan 17, 2019

do you think we may have jumped the gun with globalizing :if/:unless?

Maybe all we needed as a solution for #124 was an :ignore_nil option? It can be on both the field level and on the configuration level?

@AllPurposeName
Copy link
Contributor

AllPurposeName commented Jan 18, 2019

@philipqnguyen I don't think we've jumped the gun, nor do I like the field_name in options workaround. This solution adds bloat-free useful functionality. Doing that workaround would be a little bit messy and definitely a little bloaty. Also, overloading options is always a slippery slope.

On a positive note, I have full confidence that we could move to 1.0.0 anytime and be fine adhering to semver with a bit more caution/maturity. Alternatively, you're right, it's not a 1.0 yet so we can make changes like this. If anyone needs to, they can pin their version to 0.10.0

end
```

The field-level setting overrides the global config setting (for the field) if both are set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking: This mention might be worth a bolding.

attr_accessor :generator, :method, :sort_fields_by
attr_accessor :generator, :if, :method, :sort_fields_by, :unless

VALID_CALLABLES = %i(if unless)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we .freeze this array?

Suggested change
VALID_CALLABLES = %i(if unless)
VALID_CALLABLES = %i(if unless).freeze

config = Blueprinter.configuration

# Use field-level callable, or when not defined, try global callable
tmp = if options.key?(option_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking: I know you didn't make it, but I don't like this variable name. since it's only invoked with :if or :unless I would be for renaming it to condition or additional_condition.

@@ -1,3 +1,3 @@
module Blueprinter
VERSION = '0.10.0'
VERSION = '0.11.0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending the conversation this may become 1.0.0

@philipqnguyen
Copy link
Contributor

Ok, I'm good with this becoming 1.0.0 =) @mcclayton @AllPurposeName

@mcclayton
Copy link
Contributor Author

Thanks for all the valuable feedback @AllPurposeName and @philipqnguyen.
I agree that I think this feature is not jumping the gun, however, I think that the breaking change is.
I can remove the field_name option from this PR so that there is no breaking change, then what I'd like to do is cut a release candidate branch for 1.0.0-rc where we can group a number of breaking changes, such that we can group our breaking changes together for the 1.0.0 release.

@AllPurposeName
Copy link
Contributor

A release candidate branch is a great compromise. I look forward to all the breaking changes we add to it.

@philipqnguyen
Copy link
Contributor

lgtm

@mcclayton mcclayton merged commit f2415a3 into master Jan 18, 2019
@jmeridth jmeridth deleted the globalIfUnlessProcs branch July 26, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants