sdk
sdk copied to clipboard
Consider explicit `__all__` declarations to reduce breakages on internal refactors
https://github.com/meltano/sdk/compare/v0.22.0...v0.22.1#diff-c756d9cb24f5120280e89fe6b02f14467d23cf6c39be627bd68f9c1e5f1cf095L31-R34
Here's a change we didn't expect to break any targets, but moving the import Sink operation into a type-checking only mode did break a target that was previously importing Sink from target_base.
While it's correct that the better place to import Sink from is the sinks module or similar, we don't as of now have an explicit declaration of what should or shouldn't be imported from each module. This means there are many ways developers can import class definitions that work today and might not work tomorrow.
Two things we can do to mitigate, I think:
- A standard way to mitigate this is to explicitly declare
__all__in our modules, to dissuade type helpers from promoting an import from modules which are not the primary/expected import source. (I.e., as in the above example, iftargets_base.pyneeds to importSinkclass for its own work, that doesn't imply we want developers to importSinkfromtargets_base.) - We also should consider any changes to these importable references to be breaking changes for developers. And these should always trigger a minor version bump, with notification in release notes. In this case, I don't think we expected any breakage, but in future if we have explicit
__all__declarations, the API interface changes would be explicit and more easily notable to us as things we'd message about in release notes.
We might be able to check for 1 consistently if https://github.com/charliermarsh/ruff/issues/3482 is implemented
@edgarrmondragon - Brilliant! Yes, great idea. Let's hold for now and plan to implement when that feature releases.
Related: I still haven't updated VS Code to use Ruff - could a good topic and demo to do in Office Hours next week or the following week? Wdyt?
@aaronsteers I'd recommend the official Ruff VS Code extension: https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff
It provides commands to autofix all issues in the current file, and to sort imports. These don't have any keybindings by default, but keybindings can be given to them.
It also provides yellow underlines when it detects an issue, and adds context menu options for those to auto-fix the issue if possible, or to add a properly scoped noqa comment otherwise. It's pretty great!
This has been marked as stale because it is unassigned, and has not had recent activity. It will be closed after 21 days if no further activity occurs. If this should never go stale, please add the evergreen label, or request that it be added.
Considering trying griffe to warn ourselves about breaking API changes, in CI
I'll vouch for griffe and its author. All of my interactions with him previously have been fantastic, and the tool has been high quality in my experience. I've used it under mkdocstrings, and had to work directly with griffe a fair bit since I was trying to get it to work on a Cython project, which it wasn't originally designed to do.
@WillDaSilva Nice! Thanks for the feedback. I've explored griffe a bit more and it does look like the right tool. It's probably gonna be tough to prioritize, maybe a good idea for Hackday.
Griffe now supports GitHub output under the Insiders sponsorware strategy: https://mkdocstrings.github.io/griffe/insiders/#1000-gravifridge-user-manual
https://github.com/mkdocstrings/griffe/issues/192#issuecomment-1974822484