Skip to content
This repository was archived by the owner on Jun 15, 2021. It is now read-only.

Allow non-string types for applicant attributes#49

Merged
zachmargolis merged 1 commit intomainfrom
margolis-boolean-attribute
Feb 4, 2021
Merged

Allow non-string types for applicant attributes#49
zachmargolis merged 1 commit intomainfrom
margolis-boolean-attribute

Conversation

@zachmargolis
Copy link
Contributor

@zachmargolis zachmargolis commented Feb 3, 2021

Context:

  • I introduced a boolean applicant attribute as a feature flag

Problem:

  • The "empty?" check we do blows up on non-string types
  • Spec failure looks like:
  • 1) Proofer::Base#validate_attributes when optional attributes are booleans does not raise
       Failure/Error: expect { subject }.not_to raise_exception
       
         expected no Exception, got #<NoMethodError: undefined method `empty?' for true:TrueClass> with backtrace:
           # ./lib/proofer/base.rb:96:in `blank?'
           # ./lib/proofer/base.rb:65:in `block in validate_attributes'
           # ./lib/proofer/base.rb:65:in `select'
           # ./lib/proofer/base.rb:65:in `validate_attributes'
           # ./spec/lib/proofer/base_spec.rb:109:in `block (3 levels) in <top (required)>'
           # ./spec/lib/proofer/base_spec.rb:140:in `block (5 levels) in <top (required)>'
           # ./spec/lib/proofer/base_spec.rb:140:in `block (4 levels) in <top (required)>'
       # ./spec/lib/proofer/base_spec.rb:140:in `block (4 levels) in <top (required)>'
    

Solution:

  • Make the empty check more flexible WRT to object types

Copy link
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

:shipit:

@zachmargolis zachmargolis merged commit 9c73058 into main Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants