Hide WIP languages behind experimental flags (#172)

OpenCyphal / nunavut

Generate code from DSDL using PyDSDL and Jinja2

https://nunavut.readthedocs.io/

Other

39 stars 24 forks source link

Hide WIP languages behind experimental flags (#172) #173

Closed edwinbalani closed 3 years ago

edwinbalani commented 3 years ago

This is an attempt at solving #172.

Summary:

Add CLI argument --experimental-language / -Xlang. Use like nunavut -l cpp -Xlang cpp [...other options].

Design choices I made, which are open to critique:

That languages are assumed experimental by default, until we set experimental = False for them in properties.ini
Keeping the door open for adding other experimental feature-kinds in future, by leaving the CLI arguments longer than they could be. I'm thinking we could have an --experimental-foo/-Xfoo down the line.
Taking advantage of argparse's nargs option, which is a bit cleaner than taking one comma-separated list of experimental languages (which we'd have to parse ourselves). This matches the kind of format you see in compilers too, e.g. clang or gcc's -I, -W, -f option sets, which would be a nice alignment given nunavut's adjacency as a build pipeline tool.

Closes #172.

thirtytwobits commented 3 years ago

This is really great! The one thing I'd say is it'd be nicer to have a single --allow-experimental-languages switch so we didn't have to specify languages twice. The logic for handling this would be (in psedo code):

if language is experimental and "allow experimental languages" is false then:
    raise exception -> "this is an experimental language. Enable experimental languages to use it anyway. 
                                     It may change. You have been warned"

edwinbalani commented 3 years ago

Thanks!

The one thing I'd say is it'd be nicer to have a single --allow-experimental-languages switch so we didn't have to specify languages twice.

I was on the fence about that, but no strong feelings so let's go with the above. I think "you have been warned" has the brevity I couldn't find.

While trying to make the failing tests pass again, I've looked at LanguageContext again though. I'm not happy with my first attempt at the logic in there. At the moment the experimental flag is only considered in __init__, and only when a target language is specified up front (and I now know there are other ways to get languages out of the object, in later calls to get_language etc.).

What are your thoughts on this approach instead: the contents of the LanguageContext._languages dict shall differ, based on whether the LanguageContext was initially created with allow_experimental_support=True or ...=False? So in general, the 'view' of the available languages presented by the class will be restricted if it's created with the experimental flag off.

It's getting late in my timezone, so either way I will now revisit this tomorrow and try to drop into the dev call.

thirtytwobits commented 3 years ago

'view' of the available languages presented by the class will be restricted if it's created with the experimental flag off.

Yeah, I think I like this better even. I'm fine with experimental language being simply "invisible" unless --enable-experimental-languages is provided.

thirtytwobits commented 3 years ago

Any chance to finish this PR today or tomorrow @edwinbalani ? We're pushing for a v1 release on Wednesday.

edwinbalani commented 3 years ago

Any chance to finish this PR today or tomorrow @edwinbalani ? We're pushing for a v1 release on Wednesday.

Hi @thirtytwobits - it's been a busy ten-ish days for me elsewhere, but I'm picking this up again now.

edwinbalani commented 3 years ago

Closing in favour of #177.