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

Support for optional attributes #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

britishtea
Copy link

This commit introduces support for optional attributes. It avoids
silently assigning the value nil to attributes that are not
present in the original input, but are attributes of the validator
object

Consider the following, similar to the example in the README:

post.inspect
# => { :title => "The Original Title", :author => "britishtea" }

class UpdatePost < Scrivener
  attr_accessor :title
  attr_accessor :author

  def validate
    if assert_present :title
      assert_length :title, 3..150
    end
  end
end

input = UpdatePost.new({ :title => "A Changed Title" })
input.valid?
# => true

This is correct. No assertions for :author are made, so no errors
are expected. When using the output of #attributes however, data
loss is very easy to occur.

post.update_attributes(input.attributes)
post.inspect
# => { :title => "A Changed Title", :author => nil }

That's because #attributes assigns nil for "missing" attributes.

input.attributes
# => { :title => "A Changed Title", :author => nil }

This commit changes #attributes to not assign nil to "missing"
attributes, so they become optional.

input.attributes
# => { :title => "A Changed Title" }

It's still possible to make assertions on optional attributes:

class UpdatePost < Scrivener
  attr_accessor :title
  attr_accessor :author

  def validate
    if assert_present :title
      assert_length :title, 3..150
    end

    if author
      assert_match :author, /[A-Z]\w+ [A-Z]\w+/
    end
  end
end

The change is subtle, but existing code might expect missing
attributes to be `nil. If it is to be merged, I think it's wise to
bump the version to 2.0.

This commit introduces support for optional attributes. It avoids
silently assigning the value `nil` to attributes that are not
present in the original input, but are attributes of the validator
object

Consider the following, similar to the example in the README:

```ruby
post.inspect

class UpdatePost < Scrivener
  attr_accessor :title
  attr_accessor :author

  def validate
    if assert_present :title
      assert_length :title, 3..150
    end
  end
end

input = UpdatePost.new({ :title => "A Changed Title" })
input.valid?
```

This is correct. No assertions for `:author` are made, so no errors
are expected. When using the output of `#attributes` however, data
loss is very easy to occur.

```ruby
post.update_attributes(input.attributes)
post.inspect
```

That's because `#attributes` assigns `nil` for "missing" attributes.

```ruby
input.attributes
```

This commit changes `#attributes` to not assign `nil` to "missing"
attributes, so they become optional.

```ruby
input.attributes
```

It's still possible to make assertions on optional attributes:

```ruby
class UpdatePost < Scrivener
  attr_accessor :title
  attr_accessor :author

  def validate
    if assert_present :title
      assert_length :title, 3..150
    end

    if author
      assert_match :author, /[A-Z]\w+ [A-Z]\w+/
    end
  end
end
```

The change is subtle, but existing code might expect missing
attributes to be `nil. If it is to be merged, I think it's wise to
bump the version to 2.0.
@Miguelcldn
Copy link

My team and I were looking for this! We updated scrivener to 1.0 and had trouble with tests. When we filter and one attribute is not in the constructor we expect that it won't exist in the attributes returned as we consider the difference between nil (might mean clean the previous value) and non-existence (as do not update this part).

This fork resolved all our tests and the logic seems cleaner for us. Could this be merged?

@Juanmcuello
Copy link

Hi @soveran,

Did you have the chance to take a look a a this PR? Just to know if it is still considered for merging as I'm facing the same problem here.

Thanks!

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.

3 participants