guardian / typerighter

Even if you’re the right typer, couldn’t hurt to use Typerighter!
Apache License 2.0
276 stars 12 forks source link

Add new property `groupKey` to `RuleMatch` #436

Closed simonbyford closed 1 year ago

simonbyford commented 1 year ago

What does this change?

Adds a new property groupKey to a rule match which indicates how the rule should be grouped when it produces matches.

This is needed because currently all Collins Dictionary rules are confusingly grouped together by the Typewriter client as they all share the same rule ID (MORFOLOGIK_RULE_COLLINS). This motivated the creation of a new field to control grouping.

Publishing this change in isolation won't have any effect, but once this PR is merged the the Typerighter client can then be updated to use groupKey to group rules in the sidebar, instead of ruleId.

How to test

Run typerighter and composer locally, ensuring composer is pointing at the local typerighter instance as per the docs. Ensure the Collins Dictionary feature switch is enabled. Create an article, open the browser network tab, make a spelling mistake and click "Check document". When you inspect the response from the HTTP request, the JSON should contain the new property groupKey. Something like:

{
    "matches": [
            {
                "rule": {
                    "id": "MORFOLOGIK_RULE_COLLINS",
                    "category": {
                        "id": "Collins Dictionary",
                        "name": "Collins Dictionary"
                    },
                    ...
                },
                ...
                "groupKey": "MORFOLOGIK_RULE_COLLINS-sherbert"
            }
        ]
...
}
simonbyford commented 1 year ago

Code looks great. I'd recommend something to document why we're doing this for dictionary matches – this could be a comment at the point at which we're mapping to add the group key, and/or a test for the dictionary matcher to ensure that we're getting back what we expect.

Blocked on one test is failing in RuleTestingSpec.scala for an obscure reason – AFAICS it looks like we're tipped some vendor code over an optimisation threshold or some other condition, and it's decided to return a Vector rather than a List for the result variable assigned on L161.

This is possible because we've declared the return type of to be Seq, which is a supertype of both List and Vector.

Calling toList on the result should fix – happy to pair, and if anyone else knows precisely why this has happened (may be Akka specific) I'd be interested to hear it!

Thanks for the comment and the green tick @jonathonherbert. The issue with the test seemed to be that groupKey was missing from a text fixture which makes sense.

I've added a small comment to document the change and definitely agree a test would be great. Perhaps we could pair on adding one?

jonathonherbert commented 1 year ago

Oh nice, I saw List->Vector but missed the property change! Glad it works well. As mentioned out of band, we can definitely ship this now and circle back with a test.