giellatekno / neahttadigisanit

Saami dictionary webapp
Other
2 stars 2 forks source link

Wrong paradigm in detail view #40

Open trondtynnol opened 4 months ago

trondtynnol commented 4 months ago

A user has notified that NDS displays the paradigm for the wrong word in some circumstances, notably:

In saan, search for "lähteä" in fin-sms. Then click on "vueʹlǧǧed". This takes you to the article for vueʹlǧǧed, but with the paradigm of vueʹlǧǧeed (-eed not -ed). If searching for vueʹlǧǧed directly in sms-fin, it works as intended.

This seems to me to be an unintended consequence of the last fix of the lemma_match functionality. The incorrect article (vueʹlǧǧeed) is filtered out, but not its paradigm, and because the paradigm of vueʹlǧǧeed is generated before the one of vueʹlǧǧed, that is the one used. Checking the console seems to confirm this, as it shows both paradigms are generated.

It works correctly when coming from sms-fin because the e-node filtering works as intended and only lets vueʹlǧǧed through. The same is not the case for the lemma_match filtering.

Phaqui commented 1 month ago

Commit https://github.com/giellatekno/neahttadigisanit/commit/d1b41c429b4641d3fe74db5c8316f2428b9a2da4 reintroduces old code, which seems to fix the bug, at least for this example, for me, if I understand it correctly.

However, I suspect that it may break other scenarios, because where this function is called, the return value is checked explicitly for being == 0 - which will never happen with the old code, because it returns a list.

trondtynnol commented 1 month ago

This seems very good at least after initial tests. Thanks! It works for another similar example as well.

trondtynnol commented 1 month ago

And the user who reported this is very happy that it is fixed :grin:

Phaqui commented 1 month ago

Good. Do you have the other test example? I am currently experimenting with an end-to-end testing framework, and getting these kinds of examples in there, would help greatly in avoiding regressions!

Also, while this could be closed, I am currently experimenting with a different fix, that would also make the caller of the offending function make sense. It's difficult for me to believe that this is the correct fix, when this fix basically breaks the caller (makes the caller have code that doesn't make sense) - ref: the caller checks the return value for being == 0. But, as we have no tests to prove what this potentially breaks, I have no idea.

My plan now is to make these tests, and make the updated fix pass these new tests.

trondtynnol commented 1 month ago

Great that you are working on testing, that is sorely needed!

The other bug report was for the Kven adjective/noun "laatas", which is also a form of the verb "laađata". When coming from the sanat nob-fkv dictionary by e.g. searching for "mo" and clicking on "laatas", the first entry's paradigm contained two verb forms and nothing else, like this:

Skjermbilde 2024-09-06 kl  13 49 15

The correct paradigm is the one currently displayed at e.g. laatas