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

Have ChangeSet save throw a meaningful exception #807

Open
elrayle opened this issue Dec 6, 2019 · 3 comments
Open

Have ChangeSet save throw a meaningful exception #807

elrayle opened this issue Dec 6, 2019 · 3 comments

Comments

@elrayle
Copy link
Contributor

elrayle commented Dec 6, 2019

Description

ChangeSets inherit a #save method from Reform/Disposable which call #sync on the change set and #save on the resource. Since Valkyrie resources do not have a #save method, this raise undefined method exception. It would be better for this to raise a more meaningful exception that provides information on how to avoid the exception.

Rational

Provide developers with information on how to avoid exceptions.

Expected behavior

Calling change set #save method should throw...

NoMethodError (undefined method `save' for #<_ChangeSet_>)

OR something more descriptive...

NoMethodError (undefined method `save' for #<_ChangeSet_>); call #sync method on change set and then use persister to save resource

Actual behavior

Calling change set #save method throws...

NoMethodError (undefined method `save' for #<_Resource_>)

To reproduce

# frozen_string_literal: true
class Book < Valkyrie::Resource
  attribute :title, Valkyrie::Types::Set.of(Valkyrie::Types::String).meta(ordered: true)
end

# frozen_string_literal: true
class BookChangeSet < Valkyrie::ChangeSet
  property :title
end

book = Book.new
book_cs = BookChangeSet.new(book)
book.title = 'test title'
book_cs.save # NoMethodError (undefined method `save' for #<Book:0x00007fafcea8c278>)
@tpendragon
Copy link
Collaborator

👍 to the more descriptive option.

@no-reply
Copy link
Contributor

is this due to the inclusion of Reform::Form::ActiveModel? are there other methods included from Reform that will dead-end this way?

@tpendragon
Copy link
Collaborator

I don't think so. The interface for Reform is pretty small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

3 participants