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

Py runtime: Move to relative imports #4705

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

pelson
Copy link

@pelson pelson commented Sep 29, 2024

This also fixes a few from antlr4 import *, which is considered bad practice unless being done to expose an interface (https://peps.python.org/pep-0008/#imports)

My changes seem to modify generated files in antlr4.xpath. I didn't yet look at fixing the generator.

There remains some significant issues withe the namespaces being exposed in the runtime - I would be happy to follow up after this change to tighten the names being made publicly available.

@ericvergnaud
Copy link
Contributor

@pelson thanks for this. LGTM apart from my 2 comments.

@pelson
Copy link
Author

pelson commented Sep 30, 2024

Thanks @ericvergnaud - I think you forgot to hit submit review. Comments missing.

runtime/Python3/src/antlr4/xpath/XPath.py Show resolved Hide resolved
@@ -1,5 +1,11 @@
# Generated from XPathLexer.g4 by ANTLR 4.13.1
from antlr4 import *
from ..Lexer import Lexer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is generated, and generated files cannot use relative imports since they normally sit outside the antlr code base. Please revert

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted in my update

Signed-off-by: Phil Elson <pelson.pub@gmail.com>
Copy link
Contributor

@ericvergnaud ericvergnaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One remaining issue

@@ -165,7 +164,3 @@ def process(input_stream, class_lexer, class_parser):
process(input_stream, class_lexer, class_parser)
else:
print("[ERROR] file {} not exist".format(os.path.normpath(file_name)))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it is bad practice to run a module directly like this (e.g. via python antlr4/_pygrun.py). For example, if you do this on a file that has relative imports, you will get errors along the lines of ImportError: attempted relative import with no known parent package

pygrun is exposed as a console script in the pyproject.toml, meaning that if you install the runtime, you have an executable on the path called pygrun (including for editable installs).

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

Successfully merging this pull request may close these issues.

2 participants