-
Notifications
You must be signed in to change notification settings - Fork 23
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
Undo DecimalFloatValueModelElement constants moving outside of class #631
Comments
Constants belong on a module level, not in every class instance. This change was proposed while rewriting the unittests. Also in PR #627 the same is implemented for the DecimalIntegerValueModelElement. |
However I can adapt all other projects and open pull requests for these changes. |
I am not sure this is a better solution. When writing a parser with both integers and floats, the constants will clash: from aminer.parsing.DecimalIntegerValueModelElement import DecimalIntegerValueModelElement, SIGN_TYPE_NONE, PAD_TYPE_NONE It will be necessary to import them under another name, which will be likely not consistent across parser models. |
You are right. We need to find another solution. What do you think of it? |
For now, please put the constants of the float element back inside the class. I agree that constants should not be there, but right now there is no other solution that does not break way too many things - integer elements are rather common in many parsers, including parsers that are not in the public repository but were made for specific projects, which cannot simply be changed and will not work anymore after this update. Also the parser generator differentiates between integers and floats and needs a somewhat big update after this change. Due to all these incompatibilities that would be introduced we should wait for the next major version release of the aminer (i.e., v3.0.0) for such refactorings. Since it is not at all security or functionality relevant and only concerns coding style, it should be okay to wait. |
okay. I will open a new issue for this. |
The constants of the DecimalFloatValueModelElement were moved outside of the class in the development branch. This causes that existing parsers are not working anymore, in particular, parsing elements like
DecimalFloatValueModelElement('float44', value_sign_type=DecimalFloatValueModelElement.SIGN_TYPE_OPTIONAL)
. I do not think there is any good reason why this happened; the constants have to be accessible from outside and we should avoid having to change all existing parsers (this would also affect the parser generator).The error is
AttributeError: type object 'DecimalFloatValueModelElement' has no attribute 'SIGN_TYPE_OPTIONAL'
. The tests should include checks so that this error is noted if the constants are moved again in the future.The text was updated successfully, but these errors were encountered: