Skip to content

More local import statements. #497

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

Closed
ali-ramadhan opened this issue Oct 23, 2019 · 6 comments · Fixed by #591
Closed

More local import statements. #497

ali-ramadhan opened this issue Oct 23, 2019 · 6 comments · Fixed by #591
Labels
cleanup 🧹 Paying off technical debt

Comments

@ali-ramadhan
Copy link
Member

There are a couple of files (e.g. Oceananigans.jl and runtests.jl) that have these mega import statements where a lot of things are imported.

I find it hard to understand why certain things are being imported and where they are being used, so I'm thinking it might be better if we import things at the top of the file where they are being used. This means using more local import statements which I'm thinking will make it clearer where and why things are being imported and used.

@glwagner @suyashbire1: What do you guys think?

@ali-ramadhan ali-ramadhan added the cleanup 🧹 Paying off technical debt label Oct 23, 2019
@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Oct 23, 2019

I personally find this ordering of import statements to be clear:

  1. Standard library imports
  2. Third-party imports
  3. Application-specific imports

It's pretty popular with Python and is apparently part of the CIA coding guidelines: https://stackoverflow.com/a/42701457

@glwagner
Copy link
Member

I'm a little conflicted about this.

In python the convention is to put import statements at the top of a file? So the python convention is not to have local import statements. Since every file is a module, this means that in python import statements are at the top of modules by convention.

In julia modules can be split among many files. Thus if imports are at the top of a file, they are scattered within the module.

One reason to put things at the top level is so that its easy to see at a glance what a package imports. It's also nice to see what methods are being extended via implication; for example if one sees import Base: + we can expect that + is redefined.

However, I understand that it is not easy to connect an import statement at the top of a massive module (like ours) with where it is used among the many files that comprise the code of a module.

I think having more submodules (#495, #456) is really the right solution to this problem; in that case each module is short so the issue of tracing import statements to usage within a huge module is alleviated.

What is the problem that this change would solve? If one is reading code in a file, and an unfamiliar function appears, one presumably looks to the top-level to see where it is imported. If one is reading a module, why is it crucial to know where in the code the package is used?

This new convention could create issues in which functionality from a package imported in one file is used elsewhere, making it hard to trace exactly where an unfamiliar function first arrived in the namespace. Or, if two packages conflict with one another. In julia, resolving method conflicts when two packages export the same name can be tricky and the resolution depends on the order in which packages are imported.

@glwagner
Copy link
Member

I definitely agree with the ordering of import statements that is proposed.

@ali-ramadhan
Copy link
Member Author

In python the convention is to put import statements at the top of a file? So the python convention is not to have local import statements. Since every file is a module, this means that in python import statements are at the top of modules by convention.

In julia modules can be split among many files. Thus if imports are at the top of a file, they are scattered within the module.

I meant "more local" so putting import and using statements at the top of each file.

I think putting them at the module level makes module files messy/cluttered and actually makes it harder to see where and how imported functions are used and overloaded.

One reason to put things at the top level is so that its easy to see at a glance what a package imports. It's also nice to see what methods are being extended via implication; for example if one sees import Base: + we can expect that + is redefined.

If I want to check what a package imports, I will check the Project.toml. I think it's very rare for me to check "what a module imports".

The much more common case in my experience is I'm looking at a file and see an unfamiliar function so I scroll to the top to see where it was imported from. If the import statement is in the module file, then I have to scroll through a screenful of import statements in another file in search of the relevant line.

What is the problem that this change would solve? If one is reading code in a file, and an unfamiliar function appears, one presumably looks to the top-level to see where it is imported. If one is reading a module, why is it crucial to know where in the code the package is used?

It solves two problems: messy module files, and makes it easier to understand where import statements are used.

Maybe we do things differently, but I always scroll to the top of the current file to look for import statements. I've never thought to look the module top-level file.

Additionally, a list of import statements at the top of the file tell me what to expect in this file. I always skim the import statements before reading a file, although this may be Pythonic behavior haha.

This new convention could create issues in which functionality from a package imported in one file is used elsewhere, making it hard to trace exactly where an unfamiliar function first arrived in the namespace. Or, if two packages conflict with one another. In julia, resolving method conflicts when two packages export the same name can be tricky and the resolution depends on the order in which packages are imported.

We can break with convention when needed, but presumably this is the rare exception rather than rule.

I think having more submodules (#495, #456) is really the right solution to this problem; in that case each module is short so the issue of tracing import statements to usage within a huge module is alleviated.

Yeah maybe after we have more submodule it'll be clearer how to tackle this issue and maybe it won't be as bad. Although I think some refactoring/reorganizing/reordering of import statements may be needed to make the import statements more local.

@ali-ramadhan
Copy link
Member Author

Maybe it's just me, or maybe I'm still stuck in a Python mindset, but I find these screenfuls of import statements to be messy, hard to parse, and they make our code look unprofessional (in my opinion).

image

@glwagner
Copy link
Member

I guess I've just run into importing issues before when juggling many "module component" or "floating" files that are all included into a module. Then I noticed that it seems at least some julia packages seem to coalesce importing and exporting in a single location, which solved at least one of those problems. But maybe better-written code is a better solution. Looking at the Documenter.jl source, it seems the strategy there is to make every single file a module. With that organization this kind of debate disappears (and the code structure is thus identical to a more pythonic code structure).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants