if/unless option usage unintuitive

[tataihono]

Given the follow code would you expect EmailAddress.create('bob@cru.org') to be synced with GR?

class EmailAddress < ActiveRecord::Base
    global_registry_bindings if: :valid_gr_format?

    def valid_gr_format?
        (email =~ /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i).present?
    end
end

According to this: https://github.com/CruGlobal/global-registry-bindings/blob/029af37fa523504592e9d0a78ac908216f3f0e38/lib/global_registry_bindings/model/push_entity.rb#L16 it would not sync the record if the email was valid. I think this is not intuitive.

Rather if email was not valid i.e. returned false it should not be run.

This is an issue which we are experiencing in production MPDX: https://github.com/CruGlobal/mpdx_api/blob/18db62b7322cf2fbfa4b4b6f0ef693d3b3b78550/app/models/email_address.rb#L30-L46

However this would break the public API so would require an increment in the major version number.

MPDX for the time being is going to invert our if proc to work with the current gem.

[mattdrees]

Yeah, this is unintuitive to me as well. if and unless feel backwards.

[mattdrees]

@Omicron7 are these working as you intended? is if supposed to mean 'skip-gr-sync-if-this-is-true'?

[Omicron7]

I don't remember. Documentation is here https://github.com/CruGlobal/global-registry-bindings#conditional-push And specs are here. https://github.com/CruGlobal/global-registry-bindings/blob/master/spec/options/if_unless_spec.rb

I was certain it was working correctly and I believe its used by a few other projects as well.

[mattdrees]

This doc is wrong, according to your specs:

class Product < ActiveRecord::Base
  attr_accessor :should_push
  global_registry_bindings if: proc { |model| model.should_push }
end

the accessor should be called :should_not_push.

This doc is right, but it still feels odd to me to use 'unless' here.

class Product < ActiveRecord::Base
  global_registry_bindings unless: :should_push

  def should_push(_model)
    return ::GlobalConfig.gr_enabled?
  end
end

[tataihono]

@Omicron7 I think your intention was for it to be the other way around since you wrote the PR for MPDX where valid_gr_format? is meant to return true if the email is valid which should then push it to the GR.

I think there may be other projects suffering from the same issue if that is the case.