Bugfix/rrel lookup multifile problem, issue #377 #379
Conversation
…e repaired by planned code change
|
Maybe we can configure "coveralls" not to complain if we have only small changes in the test coverage? |
|
Good work! Looks good on a first look. I'll do the full review in the following days.
Good point, it is annoying to fail for negligible decrease. I increased failure threshold to 0.1%. We'll see if we need it higher. |
CHANGELOG.md
Outdated
| @@ -31,6 +31,8 @@ please take a look at related PRs and issues and see if the change affects you. | |||
|
|
|||
| ### Fixed | |||
|
|
|||
| - Fixed RREL lookup in case of multi-meta models (some special cases were not | |||
| handled correctly; [#379]). | |||
There was a problem hiding this comment.
This should go to the # Unreleased section.
CHANGELOG.md
Outdated
| @@ -555,6 +557,7 @@ please take a look at related PRs and issues and see if the change affects you. | |||
| - Metamodel and model construction. | |||
| - Export to dot. | |||
|
|
|||
| [#379]: https://github.com/textX/textX/pull/379 | |||
textx/scoping/rrel.py
Outdated
| for m in obj._tx_model_repository.local_models: | ||
| start.append(m) | ||
| if obj._tx_metamodel.builtin_models: | ||
| print("builtin!") |
There was a problem hiding this comment.
This print should be removed or commented out.
There was a problem hiding this comment.
Ah.. I missed this one. Removed,
| A r2 {} | ||
| } | ||
|
|
||
| // activate importURI (in any case, also for `GrammarMissingPlusM.tx`): |
There was a problem hiding this comment.
I added an explanation. Look if it can be understood... We may to add more verbose explanation and a hint that "+m" will be deprecated...
| // activate importURI (in any case, also for `GrammarMissingPlusM.tx`): | ||
|
|
||
| R R.r2 | ||
|
|
There was a problem hiding this comment.
Hmm. We can also remove the whole comment... Without the "R" command, we could also pass the test w/o implementing the RREL provider correctly...
There was a problem hiding this comment.
Ah, ok. I think I've figured out what are you up to. I did a slight refactoring and comment rewording. I've moved R command before A and P1. Then in R I used a non local reference to show that the R command can find its referent in both cases since the +m for R rule is not removed. Then we can clearly see that P1 can't be resolved when +m is removed. Does that looks ok?
There was a problem hiding this comment.
Yes. Very good. Thank you for the clarification. After all it is a border case.
There was a problem hiding this comment.
If that is OK I think this is ready to be squash merged.
Code review checklist
CHANGELOG.md, no needto update for typo fixes and such).