Reserved Python keywords support (Issue 105)

[javiersorianoruiz]

We (@Bit-Seq, @jordialexalar and me) have implemented a solution for the issue 105.

#105

With this change, now, the IDL file can contain reserved Python keywords and when this IDL file is translated into Python the IDLC Python plugin has been modified to add a prefix "_" to the found reserved keywords.

We have had into account all the posible declaration using IDL: structs, modules, enums, typedefs...

We have tested broadly this change. The developed and executed tests can be review in detail in this git repository:

https://github.com/javiersorianoruiz/cyclonedds-python-checkReservedKeyword

These are not the formal tests that maybe are required by cyclonedds-python project, but they have allowed us to verify that the implementation is right and have no colateral issues.

Maybe, It would be needed to develop another test batery based on pytest and integrated into cyclonedds-python project, following your testing architecteture, What do you think?, Is it needed?

[thijsmie]

Hi @javiersorianoruiz,

This looks already really nice! I have a couple things that need some attention, mostly stylistic. There is one feature you missed, we can also resolve types dynamically from the network using the XTInterpreter. The names in XTInterpreter._make_complete_struct, XTInterpreter._make_complete_union etc also need to take care with python keywords. I can also handle this myself in a followup PR, just let me know.

[thijsmie]

I think get_c_name is not very descriptive to what it actually does: escape python keywords. I think it is actually prudent to define idlpy_identifier that calls idl_identifier and then filters keywords. This simplifies your code in quite a few places since there is no modified vs non-modified handling needed.

There are a couple places where a name of something that is not a node is determined. For that you can define a filter_python_keywords function and use that inside idlpy_identifier.

There is a few non-keyword things you should also add to the filter, namely idl, annotate, types, auto, TYPE_CHECKING, Optional and dataclass since they are occuring as imports at the top of any idl python output. See also #161.

[javiersorianoruiz]

Ok, so, we understand you don't want us to follow the cxx way of implementation.

To confirm with you:

1. Modify name of function get_c_name to filter_python_keywords
2. filter_python_keywords never returns NULL, returns the original name or the original name with the prefix "_".
3. We implement a new function const char *idlpy_identifier(const void *node) which internally is going to call:

   name = idl_identifier(node) 
   name = filter_python_keywords(name)
   return  name;

About more keywords, OK, we will also review this #161 to add all the required keywords. We have filtered the Python keywords indicated in this web page:

https://www.w3schools.com/python/python_ref_keywords.asp
https://www.geeksforgeeks.org/python-keywords/

But, only for curiosity, why do we have to add other keywords that are not considered reserved?, We suppose that the Python code generated doesn't work if it contains any of the "extra" keywords, does it?

[thijsmie]

The proposed modifications sound okay to me.

As for your questions, indeed, they are not keywords but are used in the generated code:

from enum import auto
from typing import TYPE_CHECKING, Optional
from dataclasses import dataclass

import cyclonedds.idl as idl
import cyclonedds.idl.annotations as annotate
import cyclonedds.idl.types as types

[thijsmie]

Please don't leave commented code in the PR.

[javiersorianoruiz]

ha ha ha, it was an oversight, we have already deleted this printf and another ones that we have also found.

There are other two printfs that we think they are yours.

[Bit-Seq]

Another detail not mentioned above. True and False keywords are not supported, due to they are also keywords in the IDL.

The IDLC compiler returns a syntax error even before trying to convert the IDL file to Python, so IDLC does not support these words in an IDL file for any supported language.

Anyway, True and False are keywords in almost all languages, and it is rare a lot to try to define an object with those identifiers.

[javiersorianoruiz]

Hello @thijsmie

We have updated the code with your suggested changes. So you can advance in the pull request and merge process.

Everything is working properly in our testing: https://github.com/javiersorianoruiz/cyclonedds-python-checkReservedKeyword

We have pending:

About "we can also resolve types dynamically from the network using the XTInterpreter", How do we have to do this? How do we have to write the IDL file for the method XTInterpreter._make_complete_struct or XTInterpreter._make_complete_union to be executed during the translation from IDL to Python?

If you give us more information about how to reproduce this conversion, we can try to implement the change ourselves as we have done with the rest of cases.

We have added also the keywords you ask for them, so, consider closing Issue #161 if the pull request is ok for you.

[thijsmie]

I think it is okay like this! Thank you so much for the contribution. The last remaining point I mentioned about the network types is something I'll tackle later.