Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix check for RIGHT_ATTRS in dep matcher #8807

Merged
merged 4 commits into from Aug 4, 2021

Conversation

polm
Copy link
Contributor

@polm polm commented Jul 26, 2021

If a non-anchor node does not have RIGHT_ATTRS, the dep matcher throws
an E100, which says that non-anchor nodes must have LEFT_ID, REL_OP, and
RIGHT_ID. It specifically does not say RIGHT_ATTRS is required.

An empty dict for RIGHT_ATTRS is also valid, and patterns with one will be
accepted. While not normal, sometimes a REL_OP is enough to specify a
non-anchor node - maybe you just want the head of another node
unconditionally, for example.

This change just sets RIGHT_ATTRS to {} if not present. Alternatively
changing E100 to state RIGHT_ATTRS is required could also be reasonable.

Became aware of this issue while looking at #8803.

Description

Types of change

Bug fix?

Checklist

  • I have submitted the spaCy Contributor Agreement.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

If a non-anchor node does not have RIGHT_ATTRS, the dep matcher throws
an E100, which says that non-anchor nodes must have LEFT_ID, REL_OP, and
RIGHT_ID. It specifically does not say RIGHT_ATTRS is required.

A blank RIGHT_ATTRS is also valid, and patterns with one will be
excepted. While not normal, sometimes a REL_OP is enough to specify a
non-anchor node - maybe you just want the head of another node
unconditionally, for example.

This change just sets RIGHT_ATTRS to {} if not present. Alternatively
changing E100 to state RIGHT_ATTRS is required could also be reasonable.
@polm polm changed the title Fix check for RIGHT_ATTRs in dep matcher Fix check for RIGHT_ATTRS in dep matcher Jul 26, 2021
This test was written on the assumption that if `RIGHT_ATTRS` isn't
present an error will be raised. Since the proposed changes make it so
an error won't be raised this is no longer necessary.
@polm polm added the feat / matcher Feature: Token, phrase and dependency matcher label Jul 27, 2021
@svlandeg
Copy link
Member

svlandeg commented Aug 2, 2021

I personally think that E100 should just be updated and mention RIGHT_ATTRS specifically. This does seem to be the original intention, as there is an explicit unit test checking for this error to be thrown when RIGHT_ATTRS is missing.

Alternatively, a slightly different error msg could be given for each missing attribute, mentioning the exact one that's missing and potentially also a default value that can be used (instead of setting this implicitely in code).

@svlandeg svlandeg added the feat / ux Feature: User experience, error messages etc. label Aug 2, 2021
Error message now lists missing keys, and RIGHT_ATTRS is required.
@polm
Copy link
Contributor Author

polm commented Aug 3, 2021

Sounds good to me. I changed the error message to list missing attributes.

It would be good to suggest default values, but I wasn't sure how to add that without making the error too verbose.

Also removes unused key param arg.
@svlandeg svlandeg merged commit 77d698d into explosion:master Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat / matcher Feature: Token, phrase and dependency matcher feat / ux Feature: User experience, error messages etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants