Reserved python keywords#136
Conversation
In that case, I'd say it makes sense to mark this PR as draft. |
|
No, my intention was actually asking for a quick sanity check (i.e. that ksy makes sense and it indeed does prove that the problem exists), and then merging it. The problem is there, whether we expose it or not. In my opinion, having a broken test is better than keeping this hidden. And then we can gradually introduce escaping or something similar, going language by language, and this will result in getting this test to green. |
That is typically my attitude as well, but usually the spec (often generated from KST) can be created in advance and does not have to change when the implementation in KSC arrives. But I guess it doesn't make much difference if that's not possible. |
* Accessing keyword-named struct fields in keyword-named type 'false' (especially meaningful for Python, as True/False class names generated by default are the only ones that will be clashing with reserved capitalized booleans). * Accessing enums as value instance experssions (+`.to_i` operation on them).
9d974e6 to
808cd39
Compare
| self.assertEqual(r.keyword_enum, reserved_python_keywords.ReservedPythonKeywords.True.and) | ||
| self.assertEqual(r.keyword_nested_enum, reserved_python_keywords.ReservedPythonKeywords.Def.Try.except) |
There was a problem hiding this comment.
Actually, we wouldn't have to worry about collisions between enum member names and keywords if we followed the usual Python naming convention, which is all-uppercase (unlike any Python keyword), see e.g. https://docs.python.org/3/howto/enum.html:
from enum import Enum
class Color(Enum):
RED = 1
GREEN = 2
BLUE = 3It would be a breaking change, but if we want to do it, perhaps it's better to do it sooner than later?
There was a problem hiding this comment.
https://docs.python.org/3/howto/enum.html also recommends this naming explicitly:
Note: Case of Enum Members
Because Enums are used to represent constants, and to help avoid issues with name clashes between mixin-class methods/attributes and enum names, we strongly recommend using UPPER_CASE names for members, and will be using that style in our examples.
There was a problem hiding this comment.
I think going forward we will try to offer customization options — e.g. if somebody really wants KS to generate class names with a prefix/suffix, or break typical language conventions for some reason (or conventions don't exist) and generate whatever scheme they want — they should be able to.
But, no matter what these customizations would be, the main thing should be unchanged: we'll strive for generated code to be syntactically correct.
We can adjust the defaults after we'll implement such customization options — that will be likely less painful for everybody.
There was a problem hiding this comment.
I think going forward we will try to offer customization options — e.g. if somebody really wants KS to generate class names with a prefix/suffix, or break typical language conventions for some reason (or conventions don't exist) and generate whatever scheme they want — they should be able to.
Is this really necessary? I don't remember any user requesting that. People obviously complain when the generated code is syntactically invalid due to keyword collisions, but if the code works, I don't think naming style is important enough for anyone to have to customize it (maybe sometimes when some linter enforces a specific naming convention, but that's usually the typical one widely used within the programming language community).
if somebody really wants KS to generate class names with a prefix/suffix
Why? I think it's best when the names in the generated code reflect the .ksy names as closely and unsurprisingly as possible (which is also why I dislike the idea of adding numerical suffixes to user identifiers as I explained in kaitai-io/kaitai_struct_compiler#320 (comment)).
The only potential case where I would see the point of this is in languages such as C, which do not provide any form of namespaces, so using a common prefix is the standard way to prevent name collisions.
But that's certainly not the case of Python. In fact, we already offer customizing the Python namespace using the --python-package option, as explained in https://doc.kaitai.io/lang_python.html. I really don't see any reason why we should allow users to define their own "mangling scheme", and my personal guess is that no one would use it anyway.
…t to match the binary file
Adds a test which showcases problems associated with usage of Python keywords and resulting exposure of raw, unescaped keywords into generated code.
This test is obviously broken now — both in terms of parser library that KS compiler generates (because it puts unescaped keywords as field/method/class names) and in terms of spec generated from KST (because it uses same underlying logic borrowed from KS compiler, thus unescaped too).
In future, the plan is to:
reserved_$LANGUAGE_whatever.ksy)reserved_python_builtins,reserved_python_kaitaistruct, etc)