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

Use absolute imports #283

Merged
merged 6 commits into from
Apr 20, 2022
Merged

Use absolute imports #283

merged 6 commits into from
Apr 20, 2022

Conversation

jasonpaulos
Copy link
Member

@jasonpaulos jasonpaulos commented Apr 19, 2022

Lately our relative imports have been a source of confusion/problems, and as of #273, absolute imports are now used by test code, so we might as well make the switch to absolute imports everywhere for consistency.

Though if there are objections, feel free to voice them.

Of particular note is the CONTRIBUTING.md file, in which I documented for the first time in this repo how our module and import conventions should work.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved

#### In Runtime Code

When a runtime file in this codebase needs to import another module/file in this codebase, you should import the absolute path of the module/file, not the relative one, using the `from X import Y` method.
Copy link
Contributor

@tzaffi tzaffi Apr 19, 2022

Choose a reason for hiding this comment

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

Do you want to also allow for the possibility of importing the module without from, e.g. import mypkg.sibling and import mypkg.sibling as sister? (I got the first example from pep-8)

Copy link
Member Author

Choose a reason for hiding this comment

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

For simplicity, I'd say no, let's not allow that, but I'm open to revisiting if there's a motivating use case.

Copy link
Contributor

@tzaffi tzaffi Apr 20, 2022

Choose a reason for hiding this comment

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

One motivation is for the tests use case where we use import pyteal as pt or in general, importing many objects like from X import Y1, Y2, Y3, .... , Y30 and it gets pretty gnarly.
There is one obligatory use case: avoiding namespace collisions as was mentioned in the PEP itself.

Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@jasonpaulos I'm on-board to try these options. Thanks for evaluating flake8-tidy-imports.

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Great improvement to code quality and ease of development!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants