Skip to content
This repository has been archived by the owner on May 14, 2022. It is now read-only.

Remove Multiple: False #1223

Merged
merged 1 commit into from
Apr 28, 2017
Merged

Remove Multiple: False #1223

merged 1 commit into from
Apr 28, 2017

Conversation

tpendragon
Copy link
Contributor

It's gotten hard to handle which fields are allowed to be multi-valued
and which aren't. In order to deal with conflicts and make the reasoning
much easier to handle, make everything multi-valued and handle
single-valued display via the forms (SingleValuedForm).

@@ -11,8 +13,6 @@ def notable_rights_statement?
def self.multiple?(field)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to keep overriding multiple? here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's only necessary for identifier, which needs weird behavior because of expectations in Grocer (we should fix that, but I didn't wanna cross gem boundaries for this)

Copy link
Member

Choose a reason for hiding this comment

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

OK, that's fine — I've created an issue in Grocer for this: pulibrary/grocer#24

@@ -46,7 +46,7 @@ def rights_statement
if Array(self["rights_statement"]).first.blank?
"http://rightsstatements.org/vocab/NKC/1.0/"
else
self["rights_statement"]
Array(self["rights_statement"]).first
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simplified to something like:

self['rights_statement'] || 'http://rightsstatements.org/vocab/NKC/1.0/'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like self['rights_statement'] is getting initialized to an empty string, but I can make the conditional do a bunch less casting I think.

@escowles
Copy link
Member

Looks good — there's just a couple of minor things that could be nice to clean up.

It's gotten hard to handle which fields are allowed to be multi-valued
and which aren't. In order to deal with conflicts and make the reasoning
much easier to handle, make everything multi-valued and handle
single-valued display via the forms (SingleValuedForm).
@coveralls
Copy link

coveralls commented Apr 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6f61209 on remove_multiple_false into 94d4ff0 on master.

@escowles escowles merged commit 7aaec7c into master Apr 28, 2017
@escowles escowles deleted the remove_multiple_false branch April 28, 2017 18:47
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.

3 participants