|
|
|
|
From #dspace this morning:
(11:19:49) hpottinger: hey, tdonohue, re: DCvalue, you made an interesting suggestion around 10am this morning, to just rename DCvalue as a stop-gap refactoring.
(11:21:01) mhwood: Ooh, hadn't seen that one yet. As currently written, DCValue does have a misleading name. If there is a good reason to have both simple DCValue and more elaborate MetadataValue, then renaming makes sense.
(11:21:04) tdonohue: why thank you :) Yea, IIRC, the whole "deprecation" thing was really just a "DCValue" shouldn't have "DC" in its name!
(11:21:58) hpottinger: so.... what would it take to do a simple refractoring out of the "objectionable" name, and then drop the deprecation?
(11:21:58) mhwood: OTOH DCValue is *so* simple that we might be able to just copy over a dab of code and replace it with MetadataValue.
(11:22:10) tdonohue: currently, MetadataValue & DCValue do *different things*. It might be possible to merge into one class...but, it also may not be needed. MetadataValue provides access to the "metadatavalue" DB table. Whereas DCValue is more of a "storage object" for entries in that table
(11:22:51) tdonohue: Either route is fine with me (merging into one "MetadataValue" class or just renaming)...I was mostly just noting that a simple rename may be "good enough"
(11:24:11) mhwood: Could we rename DCValue -> Metadatum (or some such) and have MetadataValue contain one instead of individual primitives?
(11:25:07) tdonohue: I had suggested renaming "DCValue" -> "MetadataEntry" (as it's really *one entry*...including information about the metadata field and the value). But, yes, that might be an approach, mhwood
(11:25:16) hpottinger: I like the idea of just refactoring out the objectionable name, and don't really care all that much about the final name, as long as the WARNPALOOZA goes away :-)
(11:25:57) tdonohue: I'll note that we also have *other* "org.dspace.content.DC*" classes. Those also have the naming issues...but surprisingly we never deprecated them.
(11:26:33) hpottinger: well, get out the red pens :-)
(11:28:26) hpottinger: the thing is, this kind of refactoring is typically the sort of "obvious change" that we'd not even bat an eye over a committer merging all on their own
(11:30:14) hpottinger: not that the refactoring is actually obvious or anything, just, if someone were to actually go and make this change on their own, I think we'd all nod our heads and say, yes.
(11:39:48) tdonohue: hpottinger++ yea, I'd agree with the change if someone just did it, and we had enough time to obviously sufficiently test it :)
(11:40:00) mhwood: OK, so what is the proper relationship between DCValue->MetadataEntry and MetadataValue? Their fields don't match up: DCValue holds "element" and "qualifier" while MetadataValue holds "fieldID" (the ID for a record in the MetadataField table).
(11:40:33) mhwood: Time is why I bring this silly problem up now. We have nearly a year until 5.0 for testing.
(11:41:46) mhwood: The time to sweep clutter off the workbench is at the start of the work.
(11:42:28) hpottinger: A simple rename of the method would be very easy (I'm tempted to just do it), and would require very little testing. The merging of metadatavlue and dcvalue is a bigger refactor, and would definitely require testing.
(11:43:22) hpottinger: if there's someone who feels strongly about merging metadatavalue, perhaps the simple refactor would be enough of a push to "get on with it"?
(11:44:23) tdonohue: DCValue is sorta an "oddity". It's mostly just a "storage container" that is created by Item.getMetadata(). Strangely, this all relates a bit to mhwood's questions about how "addMetadata()" works ;) DCValue is populated by getMetadata(), which is populated by MetadataCache.get() which queries the "metadatavalue" table.
(11:44:29) tdonohue: https://github.com/DSpace/DSpace/blob/master/dspace-api/src/main/java/org/dspace/content/Item.java#L2760
(11:44:29) kompewter: [ DSpace/dspace-api/src/main/java/org/dspace/content/Item.java at master · DSpace/DSpace · GitHub ] - https://github.com/DSpace/DSpace/blob/master/dspace-api/src/main/java/org/dspace/content/Item.java#L2760
(11:45:08) tdonohue: So, DCValue is a "storage container" of values that have been pulled out of "metadatavalue" table (which is what MetadataValue represents as a class)
(11:45:30) tdonohue: But, strangely, DCValue and MetadataValue are not directly related at this time.
(11:46:58) mhwood: Yeah, that's what I've been struggling to see. And they feel as though they want to be close relatives. If we built a few methods to make it easier to use MetadataValue both ways, and carried over getField() (which just cobbles up a stringified name for the content) then we could do away with DCValue, could we not?
(11:49:09) tdonohue: mhwood: yes, I *think so*. It sounds logical to me. It's almost like "MetadataValue" is not fully integrated. The MetadataCache subclass in Item is sorta the "go between" between MetadataValue and DCValue. So, it might be possible to improve MetadataValue in such a way that MetadataCache becomes very simple, and DCValue "disappears"
(11:49:46) mhwood: Mainly we just need to provide getter/setter pairs for DCValue's fields, make those fields private to locate all the places that should be using those methods, and then copy it all into MetadataValue. We'd need a little logic in getElement(), getQualifier(), and getFieldID() to look things up in MetadataField as needed and cache them.
(11:50:18) mhwood: Hmmm, tdonohue's idea sounds good too.
(11:50:19) hpottinger: go go mhwood :-)
(11:51:16) mhwood: NetBeans may even have code to magically replace direct field references with method calls. Haven't tried this before.
(11:51:52) tdonohue: IMHO, the "MetadataCache" subclass in Item is part of the issue here. It's actually *bypassing* using MetadataValue, and instead directly queries the "metadatavalue" table (see MetadataCache.retreiveMetadata()). Then it helps to populate DCValue (via the Item.getMetadata() methods): https://github.com/DSpace/DSpace/blob/master/dspace-api/src/main/java/org/dspace/content/Item.java#L2834
(11:51:52) kompewter: [ DSpace/dspace-api/src/main/java/org/dspace/content/Item.java at master · DSpace/DSpace · GitHub ] - https://github.com/DSpace/DSpace/blob/master/dspace-api/src/main/java/org/dspace/content/Item.java#L2834
(11:52:30) mhwood: MetadataCache really needs some re-think.
(11:52:39) tdonohue: yep, agreed
(11:54:09) tdonohue: Currently we basically have MetadataValue <-> 'metadatavalue' table <-> Item.MetadataCache <-> DCValue (in terms of communication / data flow)
(11:54:30) tdonohue: obviously that just looks *wrong* :)
(11:55:37) mhwood: So, MetadataValue should be the DAO for table 'metadatvalue', MetadataCache should cache MetadataValues, and DCValue should dissolve into MetadataValue?
(11:56:34) tdonohue: Yes, mhwood, I think that's exactly it
|
|
|
|
|
|
|
DCValue has been @Deprecated since 2007 with no plan to replace it. This causes scads of warnings on every build, obscuring real problems. We should undo the deprecation until we have some idea of how to proceed.
|
|
|
|
