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

feat: add hadd #5

Merged
merged 15 commits into from
Oct 19, 2023
Merged

feat: add hadd #5

merged 15 commits into from
Oct 19, 2023

Conversation

zbilodea
Copy link
Collaborator

No description provided.

@zbilodea zbilodea changed the title Adding hadd feature (and more changes for new name) feat: add hadd Oct 17, 2023
@zbilodea zbilodea requested a review from jpivarski October 18, 2023 07:57
@zbilodea
Copy link
Collaborator Author

Cannot figure out why it is a problem that I have "from operations.hadd import *" in the init file?

@jpivarski
Copy link
Member

from module import *

is generally scorned nowadays, like goto in C. It was a bad idea when it was added to the Python language because it limits control over what symbols are in the namespace; alternatives like

import module as m

or

import SomeClass from module

say in the import statement what new name is being added to the namespace. Importing * brings in a bunch of names that might even replace some names you intended, like

x = 123
from module import *

might replace x, if there was an x in the module. The set of names in a module is often not controlled very well, either, because the implementation might import some standard libraries (e.g. it might import math as m to use the math module, and now there's a variable named m that would get passed through from module import *).

Since the purpose of a linter is to discourage bad choices, it's telling you to not do that. If you have some particular classes or functions to copy from the odapt.operations.hadd.* namespace into the odapt.* namespace, I'd agree that it's best to import them one at a time, by name.

However, if you disagree, you can tell ruff to stop complaining by adding a noqa comment:

from odapt.operations.hadd import *  ; noqa: F403

(That's what the code number for this error is for: so that you have a quick way to mute this specific error, while still checking for everything else.) It's also possible to tell ruff to not check for certain types of errors in certain files in the pyproject.toml (I think).

If you copied this import * from Uproot or Awkward, then we're probably controlling the set of names in the files some other way. We might be setting an __all__ tuple in the module being imported—only names in that tuple get exported with import *. The reason might have been because then we control the list of names closer to where they've been defined. In the awkward.operations module, we have a very large list of submodules, and each exports (usually) only one name. Moreover, they first get exported from the submodules into awkward.operations and then from there into awkward.*. There would be a lot of repeated lists of names if we imported them individually.

I don't think that applies to your situation here: you probably have only one or two functions in odapt.operations.hadd.* to import into odapt.*, so you can do that in __init__.py and control it from there.


By the way, most packages nowadays encourage a two-letter abbreviation for themselves, like

import numpy as np

and

import awkward as ak

Whether people use it depends on how much they think other people use it as a convention, and that can be encouraged by documentation. Odapt shortens nicely to od (odd), don't you think? This two-letter convention is not widely used, apart from the now-obsolete OrderedDict. (Python 3.6+ doesn't need OrderedDict because dict is ordered by itself.)

@zbilodea
Copy link
Collaborator Author

Okay, thank you! I will do as the linter says, yes there are few functions so it's easy (though I still have to add noqa comments because it considers them unused inputs...hope that is normal), and definitely about the import odapt as od, I will add it to the docs!

@zbilodea zbilodea removed the request for review from jpivarski October 19, 2023 07:14
@zbilodea zbilodea merged commit df45547 into main Oct 19, 2023
13 checks passed
@zbilodea zbilodea deleted the feat-add-hadd branch October 19, 2023 07:14
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