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

Prioritise higher index preferred rules #40

Open
kpainter-atl opened this issue Feb 5, 2020 · 5 comments
Open

Prioritise higher index preferred rules #40

kpainter-atl opened this issue Feb 5, 2020 · 5 comments
Assignees

Comments

@kpainter-atl
Copy link
Contributor

As noted in #20

identifier: IDENTIFIER;
binary_operation: identifier 'op' operand;

If we have binary_operation and identifier as preferred rules, we'll only every receive binary_operation as a candidate as the rule stack is searched from lowest to highest index.

Was this a deliberate decision? I think it would make sense to traverse the stack from highest to lowest as it would allow consumers to target more specific rules, and if need be they can inspect the rule stack for parent rules.

@mike-lischke
Copy link
Owner

Yes, this was a deliberate decision. Otherwise you would constantly get "low level" tokens, like identifier where you want to get e.g. class name (which consists of identifiers). That might not always lead to the desired result, but I see no other approach without compromising the original idea.

@jasonf20
Copy link

jasonf20 commented Feb 5, 2020

I've solved this issue in my code by reversing the order like @kapinter-atl suggested. However in some cases I do prefer getting the parent so I've added "rulePriorities" configuration which I use after the fact to traverse the parents and if there is a parent with a higher priority I return that.

Most of the time however, the "wanted-rules" I set are not low level like "identifier", I would create a specific rule like: field_name: IDENTIFIER; and set the target rule to "field_name". This works well with the reverse direction.

@kpainter-atl
Copy link
Contributor Author

The class name example is interesting and I'd hope you'd be able to avoid those low level tokens by creating a rule to alias identifier and use that as a preferred rule where necessary. But it seems that regardless of high -> low or low -> high it would be difficult to accomodate both use cases.
Perhaps more granular configuration is needed as @jasonf20 mentioned. Either some form of rulePriorities or being able to specify what direction you want preferred rules to be analysed.

@mike-lischke
Copy link
Owner

Yes, that's indeed an interesting idea. Making that switchable should give everyone the best results.

@br0nstein
Copy link
Contributor

Hi @mike-lischke I have a related use case. I capture a low-level rule in my preferred rules, but I need to know all the rule lists it can appear within. I could refactor my grammar to duplicate the common rules in the hierarchy, giving them different names, so I can collect low-level-rule, low-level-rule-copy1, low-level-rule-copy2, etc, however I would prefer not doing that for maintenance reasons. Would you be open to a PR that adds a new configuration (e.g. collectAllPreferredRuleLists) that controls whether for each preferred rule a single ruleList or all possible ruleList's are returned? It could be done in a backwards-compatible fashion by adding an optional additionalRuleList field of CandidateRule.

This does intersect with some use cases of the translateRulesTopDown setting. I suspect some people would be leaving translateRulesTopDown = true today because that allows them to capture all possible higher level preferred rules despite them sharing a common lower-level preferred rule descendent. In the future we might want to deprecate that setting in favor of always translating the rules bottom up, and to still be able to capture all desired higher-level rules they could use this collectAllPreferredRuleLists setting and add code to their application to analyze the rule lists to find whether the preferred rule has the desired higher-level rule in one of the possible rule lists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants