Skip to content

Conversation

@snejus
Copy link
Member

@snejus snejus commented Dec 5, 2025

Export all previously publicly available definitions.

Once this is merged we should be good to go ahead with v1, I think.

Context

I found that fetchart plugin crashed in beets due to

ImportError while importing test module '/home/sarunas/repo/beets/test/plugins/test_art.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../.local/share/uv/python/cpython-3.10.18-linux-x86_64-gnu/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/plugins/test_art.py:40: in <module>
    from beetsplug import fetchart
beetsplug/fetchart.py:30: in <module>
    from mediafile import image_mime_type
E   ImportError: cannot import name 'image_mime_type' from 'mediafile' (/media/poetry/virtualenvs/beets-yAypcYUQ-py3.10/lib/python3.10/site-packages/mediafile/__init__.py)

@snejus snejus requested review from JOJ0 and sampsyo December 5, 2025 04:53
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.26%. Comparing base (a602f02) to head (5807412).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   93.26%   93.26%           
=======================================
  Files          16       16           
  Lines         817      817           
  Branches      118      118           
=======================================
  Hits          762      762           
  Misses         35       35           
  Partials       20       20           

@semohr
Copy link
Contributor

semohr commented Dec 7, 2025

As I mentioned indirectly in #86 (comment), I’m strongly against re-adding all exports from the original monolithic module.

We bumped the major version specifically to define and narrow the public API. Re-exposing legacy imports would effectively commit us to supporting all of those classes and functions indefinitely, which would severely limit our ability to refactor or redesign the internals.

So this is a firm NO from my side.

@snejus
Copy link
Member Author

snejus commented Dec 7, 2025

I am fundamentally against breaking functionality for people who depend on mediafile for the sole reason of internal restructuring - when all we have to do is to simply include the previously available API in the __all__ definition.

Please search GitHub and you will find that multiple projects import these classes/functions that I have re-added. I don't understand what is the drawback for us simply including them in __all__ as I've done it in this PR?

@semohr
Copy link
Contributor

semohr commented Dec 7, 2025

The main point is that the whole reason we bumped to 1.0.0 was to define and enforce a stable public API. By re-adding internal or legacy exports to __all__, we are effectively committing ourselves to support every class and function that previously existed, even those that were never intended for public use. That severely limits our ability to refactor, redesign, or improve the internals without breaking downstream users in the future.

Yes, some projects currently import these legacy items, but they were never guaranteed to be stable. Changing an import path or deprecating these internal functions is exactly what a 1.0.0 release is for: it’s the first version where we define what is public and what isn’t. In other words, breaking internal imports is reasonable and expected at this stage.

Re-exposing everything in __all__ might seem like a minimal change now, but it effectively freezes a large portion of our codebase in its current form forever. That tradeoff is much more damaging than the one-time migration effort required for consumers to adapt to the new, clearly defined API.

Take for instance #97: if we exported MediaField in __all__, it would already require another major version. This shows exactly how exposing internals limits our flexibility and ability to evolve the codebase.

@snejus
Copy link
Member Author

snejus commented Dec 8, 2025

Can you point me to the discussion where the specifics of what constitutes a stable public API, and what counts as legacy, have been debated, before you decided to remove these imports?

I am specifically interested to see the consensus regarding downstream developers that rely on them.


Re-exposing everything in __all__ might seem like a minimal change now, but it effectively freezes a large portion of our codebase in its current form forever. That tradeoff is much more damaging than the one-time migration effort required for consumers to adapt to the new, clearly defined API.

Take for instance #97: if we exported MediaField in __all__, it would already require another major version. This shows exactly how exposing internals limits our flexibility and ability to evolve the codebase.

Can you explain how

# mediafile/__init__.py
from .fields import MediaField


__all__ = [
    ...
    MediaField,
    ...
]

has any impact on the refactoring that you linked?

@semohr
Copy link
Contributor

semohr commented Dec 8, 2025

Can you point me to the discussion where the specifics of what constitutes a stable public API, and what counts as legacy, have been debated, before you decided to remove these imports?

We regard everything defined in __all__ originally (before the refactor) as stable public API, as we agree these should stay public. Conversely, anything not exported via __all__ is treated as private, even if it was previously imported from internal paths by external code. This approach is consistent with PEP 8s conventions around public vs. private interfaces. Because many of those previously "accidentally public" imports are no longer available at the same paths after the refactor, I’ve referred to them collectively as legacy.

Generally downstream users who depend on such private or internal objects (intentionally not defined in __all__) implicitly accept that these parts of the codebase may change, move, or disappear without notice.

Can you explain how ... has any impact on the refactoring that you linked?

In short, the refactor changes the inheritance hierarchy of the MediaField classes in order to enable correct type-hint propagation for MediaFile fields. This does alter behavior in observable ways: for example, it affects isinstance checks against MediaField. If downstream code relies on the old hierarchy or on performing such checks, this is a breaking change.

Under semantic versioning, modifying class relationships in a way that breaks inheritance expectations would require a major version bump, provided that MediaField was intended to be part of the public API.

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.

4 participants