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

Is it a good idea to list imported symbols explicitly? #24

Open
kamichal opened this issue Mar 13, 2019 · 1 comment
Open

Is it a good idea to list imported symbols explicitly? #24

kamichal opened this issue Mar 13, 2019 · 1 comment

Comments

@kamichal
Copy link
Collaborator

Please help me in deciding if that change I made on #20 is correct.
It was a huge PR and it sneaked somewhere in between before I asked this question.
Test that exposes it is here:

def test_includes_rendering():
common_include = model.Include("foo", [
model.Constant("symbol_1", 1),
model.Constant("number_12", 12),
])
nodes = [
common_include,
model.Include("root/ni_knights", [
model.Include("../root/rabbit", [
common_include,
model.Constant("pi", "3.14159"),
model.Typedef("definition", "things", "r32", "docstring"),
]),
model.Constant("symbol_2", 2),
]),
model.Include("../root/baz_bar", []),
model.Include("many/numbers", [model.Constant("number_%s" % n, n) for n in reversed(range(20))]),
]
ref = """\
from foo import number_12, symbol_1
from ni_knights import definition, pi, symbol_2
from numbers import (
number_0, number_1, number_10, number_11, number_13, number_14, number_15,
number_16, number_17, number_18, number_19, number_2, number_3, number_4,
number_5, number_6, number_7, number_8, number_9
)
"""
# call twice to check if 'duplication avoidance' machinery in _PythonTranslator.translate_include works ok
assert serialize(nodes) == ref
assert serialize(nodes) == ref

Before that change python generator created such a import statement:

from foo import *
from ni_knights import *
from numbers import *

Now the imported symbols are listed explicitly. The list contains everything that is defined by this Include and each sub-Include element unless it's already imported by previous statements in this file.

from foo import number_12, symbol_1
from ni_knights import definition, pi, symbol_2
from numbers import (
    number_0, number_1, number_10, number_11, number_13, number_14, number_15,
    number_16, number_17, number_18, number_19, number_2, number_3, number_4,
    number_5, number_6, number_7, number_8, number_9
)

There is also some "duplication avoidance" mechanism that raises my doubts at most.
The code responsible for that generation is here:

def translate_include(self, include):
included = list(sorted(n.name for n in include.defined_symbols() if n.name not in self.included_symbols))
self.included_symbols.update(included)
if not included:
return ""
statement_begin = "from %s import " % include.name.split("/")[-1]
symbols = ", ".join(included)
if len(statement_begin) + len(symbols) <= 80:
return statement_begin + symbols
@import_breaker
def importer():
yield symbols
return "%s(\n%s)" % (statement_begin, "".join(importer()))

Please let me know if you would like to bring it back to the original form.

@kamichal kamichal mentioned this issue Mar 13, 2019
@aurzenligl
Copy link
Owner

Explicit listing allows to figure out which symbol is imported from which include -> not a bad thing to know. Unless there are problems stemming from being explicit - I see no problems with it.

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