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.

Sign up for GitHub

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

Jump to bottom

Look at the full collection of Exchange classes #1761

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Look at the full collection of Exchange classes #1761

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb8b274
Look at the correct collection of Exchange classes
shreve Mar 23, 2023
d98324c
Rename abc.Exchange to abc.ABCExchange for clarity
shreve Jan 3, 2024
4c51c81
Deprecate accessing Exchange classes directly from nbgrader.exchange
shreve Jan 3, 2024
fcbe7ea
Add docstrings to exchange classes for better help text
shreve Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 14 additions & 21 deletions nbgrader/apps/baseapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,35 +149,28 @@ def all_configurable_classes(self) -> TypingList[MetaHasTraits]:
if len(app.class_traits(config=True)) > 0:
classes.append(app)

def _collect_configurables(module):
"""Return a list of all configurable classes from a module"""

for name in module.__all__:
cls = getattr(module, name)
if hasattr(cls, "class_traits") and cls.class_traits(config=True):
classes.append(cls)

# include plugins that have configurable options
for pg_name in plugins.__all__:
pg = getattr(plugins, pg_name)
if pg.class_traits(config=True):
classes.append(pg)
_collect_configurables(plugins)

# include all preprocessors that have configurable options
for pp_name in preprocessors.__all__:
pp = getattr(preprocessors, pp_name)
if len(pp.class_traits(config=True)) > 0:
classes.append(pp)
_collect_configurables(preprocessors)

# include all the exchange actions
for ex_name in exchange.__all__:
ex = getattr(exchange, ex_name)
if hasattr(ex, "class_traits") and ex.class_traits(config=True):
classes.append(ex)
# include all the abstract exchange actions
_collect_configurables(exchange)

# include all the default exchange actions
for ex_name in exchange.default.__all__:
ex = getattr(exchange, ex_name)
if hasattr(ex, "class_traits") and ex.class_traits(config=True):
classes.append(ex)
_collect_configurables(exchange.default)
Copy link
Contributor

@brichet brichet Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
_collect_configurables(exchange.default)
_collect_configurables(exchange.default.exchange)

Do you think we need to catch all the default classes traits if they are similar to the abc ones ?
Since exchange is the only default class with dedicated traits, maybe it can be specified when calling the _collect_configurables function.
This should avoid duplication for most of the exchange classes, but I'm not sure it will not break anything about the functionalities, I don't know well traitlets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After digging into this more, I think the duplication is just the nature of the beast. I have a new proposal to make the distinction more clear, which is to actually rename the abc classes.

When they're used in the nbgrader codebase, we already do from exchange.abc import Exchange as ABCExchange, and if you search GitHub, so does everyone else: https://github.com/search?q=from+nbgrader.exchange+import+&type=code

By renaming the actual class, we can get rid of these import aliases without touching the implementation, and create distinction in the generated config file.


# include all the converters
for ex_name in converters.__all__:
ex = getattr(converters, ex_name)
if hasattr(ex, "class_traits") and ex.class_traits(config=True):
classes.append(ex)
_collect_configurables(converters)

return classes

Expand Down
1 change: 1 addition & 0 deletions nbgrader/exchange/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from nbgrader.exchange.abc import (Exchange, ExchangeError, ExchangeCollect, ExchangeFetch, ExchangeFetchAssignment,
ExchangeFetchFeedback, ExchangeList, ExchangeReleaseAssignment, ExchangeRelease,
ExchangeReleaseFeedback, ExchangeSubmit, ExchangeReleaseFeedback)
from nbgrader.exchange import default
from .exchange_factory import ExchangeFactory

__all__ = [
Expand Down