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

add config option: rule_ordering #52

Merged
merged 18 commits into from
Sep 8, 2020
Merged

add config option: rule_ordering #52

merged 18 commits into from
Sep 8, 2020

Conversation

eddieantonio
Copy link
Collaborator

@eddieantonio eddieantonio commented Sep 2, 2020

This adds a config option called rule_ordering, which is a more self-descriptive alias of as_is.

Why?

Say I am explaining how to write a mapping.

Rule order matters! This is how you can affect how g2p orders your rules:

  • as_is: true means your rules are applied as written
  • as_is: false means your rules are sorted such that the longest rules are applied first

I suggest changing the explanation this:

  • rule_ordering: as-written means the rules are applied in the order they are written in the mapping file
  • rule_ordering: apply-longest-first means that g2p will first sort these rules, such that rules with the longest input are applied first; this prevents them from taking part of unwanted feeding relations.

Addresses #51

@eddieantonio eddieantonio added the enhancement New feature or request label Sep 2, 2020
Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not keen on having two options that are synonyms of one-another, to be honest, but I understand the desire to rename this flag since as_is is not intuitive or self-documenting.

If we all agree rule_ordering=... is preferable, I would suggest converting all the existing mappings from as_is to rule_ordering and issuing a warning when the system sees as_is indicating it's deprecated though still supported. Or go one step further: convert all the rules and stop accepting as_is altogether, outputting instead an message telling the user how to convert correctly to the new syntax.

Furthermore, the documentation for as_is should be updated to say it means the same as the other option, and please use that one instead. And, the doc for rule_ordering should say "This was formerly done by setting as_is=False" or "as_is=True", respectively.

g2p/mappings/__init__.py Outdated Show resolved Hide resolved
eddieantonio and others added 2 commits September 4, 2020 13:18
[skip ci]: this will fails, since the tests expect a `ValueError`

Co-authored-by: Eric Joanis <Eric.Joanis@cnrc-nrc.gc.ca>
@joanise
Copy link
Collaborator

joanise commented Sep 4, 2020

Nice testing of the logging messages, I needed just that to test my changes on g2p doctor!

@eddieantonio
Copy link
Collaborator Author

It went from 3 files modified to 42 files modified with all of the as_is changed to rule_ordering! 😂

Copy link
Collaborator

@joanise joanise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, this is much better, and I am happy to have this self-documenting rule_ordering name for the option. Good unit testing - the coverage went up a notch, which is always good to see! As far as I'm concerned, you can merge this in, as long as Aidan gives his go-ahead too.

@roedoejet roedoejet merged commit e375a17 into master Sep 8, 2020
@roedoejet
Copy link
Owner

Fixes: #51

@roedoejet roedoejet mentioned this pull request Sep 16, 2020
Closed
6 tasks
@roedoejet roedoejet deleted the config-rule-ordering branch September 17, 2020 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants