wikibase: initial somevalue novalue editing support by Abbe98 · Pull Request #5360 · OpenRefine/OpenRefine

Abbe98 · 2022-10-21T07:45:45Z

Part of #1713.

This makes it possible to set novalue / somevalue statements when editing by adding two "magic" keywords #NOVALUE# and #SOMEVALUE#.

Usage:

Have a column of magic keywords
Reconciliate column
Clear reconciliation data
Use the magic keyword column in the schema editor

Known issues / remaining tasks:

~~Not all data types are supported (only properly tested with dates so far)~~
~~Statement merger doesn't work~~
~~Preview is blank~~

Implementation details:

SpecialValueNoValueException and SpecialValueSomeValueException extends SkipSchemaExpressionException, these should probably all instead extend a generic SchemaException
The "Some value" terminology is used rather than the "Unknown value" as the later is known to be confusing.
The magic keywords both ends and starts with a hash sign to avoid looking like possible fragment URIs.

wetneb

Very nice, congrats!
Do not forget to write unit tests as you go along - this part of the codebase has ample server-side tests so you should be able to imitate existing ones for most classes you touch.

Abbe98 · 2022-10-21T13:39:40Z

        if (!existingSnak.getPropertyId().getId().equals(addedSnak.getPropertyId().getId())) {
            return false;
-        } else if (existingSnak instanceof NoValueSnak && addedSnak instanceof NoValueSnak) {
+        } else if (existingSnak instanceof NoValueSnakImpl && addedSnak instanceof NoValueSnakImpl) {


@wetneb I'm not sure if this ever worked or if I have caused what makes this change necessary.

I am surprised it is necessary actually! In which situations does it make a difference to check for NoValueSnakImpl instead of NoValueSnak? In general for this library the user (us) is expected to refer to the interfaces only, not their implementations, so this is a sign that something fishy is going on.

But yes, this code was indeed not used before, since we could not have an addedSnak that would be a NoValueSnak.

In this case I never got the NoValueSnak variant to work for either addedSnak or existingSnak. Debugging indicated that only *Impl instances were passed around.

However, I think the normal value snaks were passed around correctly.

Ok! I guess the tests should help us get this to work. For the naming, the *Impl instances are also instances of the corresponding interfaces, so I really think this should not make any difference. Because the interfaces are meant to be the things we use here, I would prefer if we did not refer to the implementations.

Do you have suggestions for good tests here? I'm drawn to the JSON-blob based ones but they aren't the easiest to follow on my end.

I would write at least something in WbStatementExprTest. Just copy one of the existing test cases there (for instance testEvaluate) and replace one of the cell contents (defined in setRow) by one of your magic constants. Then, construct the expected value with the appropriate No/SomeValueSnak. That should be fairly straightforward.

Then, for the SnakOnlyStatementMerger, you could follow the same strategy: copy some existing test case, change the some/both of the involved statements being matched to have some no value / somevalue case, and specify the expected matching result.

WbStatementExprTest doesn't seem to test anything other than WbExpression as the evaluateTo method is inherited from WbExpressionTest? At best I could catch the new exceptions.

No, it does test WbStatementExpr. The evaluateExpr method can indeed work with any WbExpression, it is a utility that is used in many test classes. In this case it is used to test WbStatementExpr via a wrapper defined in the test class.

We still need to change this to use the interfaces and not the implementations, just as a matter of good practice. Note that we are defining FullyPropertySerializingNoValueSnak.java ourselves so we know those interfaces can have more implementations.

Yeah sure, problem still is that changing to the interfaces break it.

Abbe98 · 2022-10-21T17:17:01Z

@trnstlntk there are some implementation details left but it should already work as expected.

trnstlntk · 2022-11-06T17:08:11Z

I have tested it (on the user side) and it works great. One example edit: https://commons.wikimedia.org/w/index.php?title=File%3AKoninklijk_Theater_Tuscinski_%E2%80%93_cinema_organ_02.jpg&type=revision&diff=703109719&oldid=703088605

The workflow is ... "specific" but I think it makes sense.

wetneb · 2022-11-06T19:12:53Z

The workflow is ... "specific" but I think it makes sense.

Just to emphasize: stated as such the workflow is very convoluted, but this is not the intended use case: this implementation is not meant to serve the case where the added value is constantly novalue/somevalue, but rather the case when this depends on which row.

See the issue for a proposed workflow for constant novalue/somevalue (which could be added as a follow-up PR).

wetneb

Are you still working on this @Abbe98? I have had a look at the outstanding things needing to be resolved before we merge the PR, see below. As mentioned before, we also need Java tests for this.

wetneb · 2022-11-20T16:24:58Z

+  } else if (json.snaktype === "somevalue") {
+    $('<span>somevalue</span>').appendTo(container);
+  } else if (json.snaktype === "novalue") {
+    $('<span>novalue</span>').appendTo(container);


Since those strings are shown to the user, it would be good to:

use user-friendly terms instead (Wikibase displays "unknown value" / "no value")

make it possible to localize them with $.i18n

+1 I would suggest "some value" and "no value" as Commons does it.

wetneb · 2022-11-20T16:27:10Z

+            if ("#NOVALUE#".equals(cell.toString())) {
+                throw new SpecialValueNoValueException();
+            } else if ("#SOMEVALUE#".equals(cell.toString())) {
+                throw new SpecialValueSomeValueException();


Since the "#NOVALUE#" and "#SOMEVALUE#" Strings have a special meaning defined by this class, I still think it would be good to have them as public static final String in this class, to make them part of the public interface of this class. This could be reused in tests, initially.

wetneb · 2022-11-20T16:28:55Z

        if (!existingSnak.getPropertyId().getId().equals(addedSnak.getPropertyId().getId())) {
            return false;
-        } else if (existingSnak instanceof NoValueSnak && addedSnak instanceof NoValueSnak) {
+        } else if (existingSnak instanceof NoValueSnakImpl && addedSnak instanceof NoValueSnakImpl) {


We still need to change this to use the interfaces and not the implementations, just as a matter of good practice. Note that we are defining FullyPropertySerializingNoValueSnak.java ourselves so we know those interfaces can have more implementations.

wetneb · 2022-12-01T10:46:43Z

Hi @Abbe98, are you still working on this? I just started a discussion on releasing 3.7-beta1: if you plan to finish this soon and want us to wait for you, let us know: https://forum.openrefine.org/t/releasing-3-7-beta1/214

wetneb · 2022-12-02T06:58:13Z

I would merge this and release it in 3.7. I'll open a documentation PR for it too.

Abbe98 · 2022-12-02T08:27:37Z

Thanks @wetneb! 🚀

Abbe98 requested a review from wetneb October 21, 2022 07:45

Abbe98 self-assigned this Oct 21, 2022

wetneb reviewed Oct 21, 2022

View reviewed changes

Comment thread extensions/wikibase/src/org/openrefine/wikibase/schema/WbDateVariable.java Outdated

Comment thread extensions/wikibase/src/org/openrefine/wikibase/schema/WbVariableExpr.java

Abbe98 force-pushed the wikibase-mixed-somevalue-novalue-support branch 2 times, most recently from 09ca762 to 65ee36a Compare October 21, 2022 12:35

wikibase: initial somevalue novalue editing support

b3d0784

Abbe98 force-pushed the wikibase-mixed-somevalue-novalue-support branch from 65ee36a to b3d0784 Compare October 21, 2022 13:38

Abbe98 commented Oct 21, 2022

View reviewed changes

wetneb requested changes Nov 20, 2022

View reviewed changes

wetneb added 2 commits December 2, 2022 07:31

Add test and fix small issues

7b80dc3

Cleanup unused imports

368e255

wetneb approved these changes Dec 5, 2022

View reviewed changes

wetneb merged commit 1f66bdf into OpenRefine:master Dec 5, 2022

Conversation

Abbe98 commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wetneb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Abbe98 Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abbe98 Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abbe98 Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abbe98 commented Oct 21, 2022

Uh oh!

trnstlntk commented Nov 6, 2022

Uh oh!

wetneb commented Nov 6, 2022

Uh oh!

wetneb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wetneb commented Dec 1, 2022

Uh oh!

wetneb commented Dec 2, 2022

Uh oh!

Abbe98 commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Abbe98 commented Oct 21, 2022 •

edited

Loading

Abbe98 Oct 21, 2022 •

edited

Loading

Abbe98 Oct 21, 2022 •

edited

Loading

Abbe98 Oct 22, 2022 •

edited

Loading

Abbe98 commented Dec 2, 2022 •

edited

Loading