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 __all__ to advertise public API. #337

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

grahamgower
Copy link
Member

Closes #324.

Note that non-public symbols are still accessible, and that some
applications, such as iPython, will ignore __all__ when offering
tab-completion suggestions.

@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #337 (5cf248b) into main (3e53eef) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #337   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files           5        5           
  Lines        1369     1372    +3     
=======================================
+ Hits         1368     1371    +3     
  Misses          1        1           
Impacted Files Coverage Δ
demes/__init__.py 91.66% <100.00%> (+2.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e53eef...5cf248b. Read the comment docs.

@grahamgower
Copy link
Member Author

Annoyingly, people who use python notebooks to explore libraries with tab completion (instead of reading documentation) will still see, and be able to access, non-public symbols. I believe the number of people who use notebooks like this is substantial.

$ ipython
Python 3.9.5 (default, May 24 2021, 12:50:35)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.21.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import demes

In [2]: demes.<TAB>
  Admix               Deme                dumps()             load()              loads()             Pulse
  AsymmetricMigration demes               Epoch               load_all()          loads_asdict()      Split
  Branch              dump()              from_ms()           load_asdict()       Merge
  Builder             dump_all()          Graph               load_dump           ms
In [2]: demes.ms.<TAB>
        Admixture                  build_graph()              demes_sorted_by_ancestry() from_ms()                  logging
        Any                        coerce_nargs()             Dict                       GrowthRateChange           Mapping
        argparse                   copy                       Event                      List                       math                       >
        attr                       demes                      finite()                   logger                     MigrationMatrixChange
In [2]: demes.ms.coerce_nargs
Out[2]: <function demes.ms.coerce_nargs(obj_creator, append=False)>

See ipython/ipykernel#129

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks good, but I don't know why we'd do this for anything other than the top-level init.py

Also, we need a new test module that does something like this one

(The strings in all aren't resolved until run time - this is the only way to be sure they aremeaningul)

@@ -11,6 +11,17 @@

import demes
Copy link
Member

Choose a reason for hiding this comment

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

Not clear to me why we'd have an __all__ in implementation-private submodules. We woulnd't encourage users to use from demes.load_dump import *, would we?

@grahamgower
Copy link
Member Author

IMO, this is more about what the public API is being advertised as, rather than caring about specific import * behaviour. I don't think anyone advocates import * anymore, except perhaps for legacy reasons (e.g. from tkinter import *). But __all__ has become the de-facto standard list that editors use to provide tab-completion lists: __all__ implicitly corresponds to the API that library authors are making public.

@jeromekelleher
Copy link
Member

Sure, I agree noone should actually be using this, but I still don't see why you'd want to add __all__ lists to the internal modules. We actively want to hide the internal module structure from users (so that we can change it later, if we want to) not advertise it.

@grahamgower
Copy link
Member Author

In some sense you're right about not needing __all__ in internal modules like load_dump.py. But unfortunately commonly used tools like the python REPL and ipython/notebooks will blindly tab-complete the names of internal modules. So I've updated the PR to go one step further and define def __dir__(): sorted(__all__), because the python REPL and ipython notebooks don't tab-complete anything except what dir(<module-name>) shows.

A possible further step (which I don't want to take) is to delete the non-public symbols from the modules so that they can't be imported even if one knows the name (E.g. like this: https://github.com/tensorflow/tensorflow/blob/e9de087fa7f59c39bbe12ac2c83c5547c83f746c/tensorflow/python/util/all_util.py#L86).

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM - just needs a test checking import * works, like I suggested, and something covering dir

demes/__init__.py Show resolved Hide resolved
@grahamgower grahamgower force-pushed the export branch 4 times, most recently from 9e0a277 to 2ad064d Compare June 24, 2021 14:50
Closes popsim-consortium#324.

Note that non-public symbols are still accessible when referred to
explicitly by name. But here we use the module-level __dir__()
function to define which symbols are seen when using dir(<module-name>).
This is a Python >= 3.7 only feature, but the function is just ignored
on Python 3.6.
@grahamgower grahamgower merged commit 1a74244 into popsim-consortium:main Jul 5, 2021
@grahamgower grahamgower deleted the export branch July 6, 2021 06:13
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.

clean up the exported names
2 participants