-
Notifications
You must be signed in to change notification settings - Fork 97
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
Language transformer #582
Language transformer #582
Conversation
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
aede900
to
9c02947
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks like a good incremental improvement. I wish we could use a term in linguistics for this concept of "transformation", but AFAIK there is not one general enough to capture what we want (beyond just deinflections), so I guess this is fine.
For what it's worth, I did do a little bit of research to see if there were standard terms for what I am describing, but I couldn't find anything beyond more generic than (de)inflection. |
Originally discussed a bit in #581, this is an initial pass of generalizing and extending the deinflector a bit.
With regards to naming, you can see that I have currently gone with
Condition
,Transform
, andRule
for the naming, all of which don't share any suffix and I think still get the point across, but their usages is a bit different than how the old deinflector used terms.I think this is a bit better because
Condition
is now named how it is actually used (formerly this was a "Rule"), i.e. that it is used as a condition to test whether aTransform
'sRule
will match. I'm still open to thoughts on this, but so far I like this paradigm as it avoids the need to have<something>Group
and similar (pre|suf)fixed constructs.This PR now has parity with the old
Deinflector
. It does not fully support all of the longer-term features I mentioned before as I consider that to be out of scope for a first PR. Mainly, it does not support internationalization or proper combination for loading multiple descriptors.As a side note, it should be relatively easy to support deinflection of multiple languages simultaneously. While there is currently a limit on the number of condition flags (32), each language should be able to use its own set of condition flags. Adding support for this was again outside the scope of this PR, but it would be easy to create a
MultiLanguageTransformer
component.