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

Built-in NUMBER can't return INT type #97

Closed
approxit opened this issue Nov 8, 2018 · 4 comments
Closed

Built-in NUMBER can't return INT type #97

approxit opened this issue Nov 8, 2018 · 4 comments

Comments

@approxit
Copy link

approxit commented Nov 8, 2018

Textx version:

1.8.0

Python version:

3.6.5

Minimal example:

import textx

mm = textx.metamodel_from_str('''
    Model:
      value1=NUMBER value2=NUMBER
    ;
''')

m = mm.model_from_str('''
    10 1.5
''')

print('Value1:', repr(m.value1))
print('Value2:', repr(m.value2))

Expected results:

Value1: 10
Value2: 1.5

Actual Results:

Value1: 10.0
Value2: 1.5

Problem:
As example shows, NUMBER built-in rule returns always float value. As NUMBER built-in rule is not documented, and not discouraged in source code, there is not obstacle to use it.

It matches FLOAT first, then INT as it's definition stands.

Any proper INT can be parsed as correct FLOAT number, so by using NUMBER with FLOAT | INT order, we end up with FLOAT result, which is not expected.

Proper order and mention in docs about NUMBER would be nice.

@goto40
Copy link
Member

goto40 commented Nov 12, 2018

Thank you for your analysis!

The problem is/was, that a FLOAT also pares an INT. With the parser we employ we have an issue here.

We discussed the problem and introduced a STRICTFLOAT (which does not match an INT) as workaround. A number is now an INT or a STRICTFLOAT.

The original FLOAT rule remains unchanged (but is not a NUMBER any more). We adapted the docu to make the changes clear.

(Details: see #98)

With the current master your test works (a unittest was added: https://github.com/igordejanovic/textX/blob/master/tests/functional/regressions/test_issue97.py).

Does this change solve your problem sketched above?

@approxit
Copy link
Author

That's awesome, guys! STRICTFLOAT would be actually handy, nice to have this one.
..and my 5-second-idea for only changing parsing order was totally wrong...

I could close this issue, but current state of docs is confusing for me.
As @igordejanovic mentioned, current images of NUMBER is not reflecting the actual "inheritance". I personally would stick to keep FLOAT next to the NUMBER instead of mixing (STRICT)FLOAT. Keeping it separate is more clear for me.

And there are two images that should be updated to keep everything in sync:
https://github.com/igordejanovic/textX/blob/master/docs/images/hello_meta.dot.png
https://github.com/igordejanovic/textX/blob/master/docs/images/robot.tx.dot.png

@goto40
Copy link
Member

goto40 commented Nov 13, 2018

Thank you for your in-depth review! This really helps getting the things right.

Maybe you could have a look at #100 to see if the changes described there are clear to you. I greatly appreciate any further hints...

@approxit
Copy link
Author

Everything is working now, thanks guys!

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

No branches or pull requests

2 participants