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

Fixing issue from PR #128. Str. match in abstract rule. #134

Merged
merged 4 commits into from Dec 9, 2018

Conversation

igordejanovic
Copy link
Member

Fixes issue discovered in PR #128.

While I was there two bare except were fixed also.

@goto40
Copy link
Member

goto40 commented Dec 8, 2018

Good work! It is great that textx is such a "defined" project. The unittests and the clarity of the code allows efficient analysis of observed effects and easy fixes!

I have a question about rules of the style "A: B| '(' C D ')'. Should such a "partly specialization" be disallowed and yield a grammar error? What should this meta model describe? A is a base of (C and D) together.... (???). See example (I pasted it at the end of the new regression test):

I could imagine that if we throw a grammar error if more than one rule is combined in such a case is maybe a good idea? Do you have an idea about the impact (someone uses something like this already?)?

def test_abstract_alternative_multiple_rules():
    grammar = r'''
    Model: a+=A;
    A: B | '(' C D ')';
    B: 'B' name=ID x=INT;
    C: 'C' name=ID;
    D: 'D' x=INT;
    '''

    mm = metamodel_from_str(grammar)

    modelstr = '''
    B b1 1
    B b2 2
    ( C c1 D 11 )
    ( C c2 D 12 )
    '''

    model = mm.model_from_str(modelstr)

    assert model.a[0].name == 'b1'
    assert model.a[0].x == 1
    assert textx_isinstance(model.a[0], mm['A'])
    assert textx_isinstance(model.a[0], mm['B'])
    assert not textx_isinstance(model.a[0], mm['C'])
    assert not textx_isinstance(model.a[0], mm['D'])

    assert model.a[2].name == 'c1'
    #assert model.a[2].x == 11                       # does not work
    assert textx_isinstance(model.a[2], mm['A'])
    assert not textx_isinstance(model.a[2], mm['B'])
    assert textx_isinstance(model.a[2], mm['C'])
    #assert textx_isinstance(model.a[2], mm['D'])  # fails

Copy link
Member

@goto40 goto40 left a comment

Choose a reason for hiding this comment

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

I would appreciate to clarify the use of rules like in the example (A:B| C D).

@igordejanovic
Copy link
Member Author

Good point. I think that A: B | '(' C D ')'; should rise a grammar error during meta-model construction as C D combination doesn't make much sense to me. I'm not aware of any grammar that uses something like this.

@igordejanovic
Copy link
Member Author

This PR has been cleaned up. I did a force push so please update your local branch. Furthermore, referencing multiple rules from a single choice of abstract rule is now explicitly disallowed by raising TextXSemanticError.

@goto40
Copy link
Member

goto40 commented Dec 9, 2018

Great!

@goto40
Copy link
Member

goto40 commented Dec 9, 2018

Ready to merge...

@igordejanovic igordejanovic merged commit 20b2f76 into master Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants