sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Consider explicit `__all__` declarations to reduce breakages on internal refactors

Open aaronsteers opened this issue 2 years ago • 8 comments

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:

  1. 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, if targets_base.py needs to import Sink class for its own work, that doesn't imply we want developers to import Sink from targets_base.)
  2. 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.

aaronsteers avatar Mar 30 '23 17:03 aaronsteers

We might be able to check for 1 consistently if https://github.com/charliermarsh/ruff/issues/3482 is implemented

edgarrmondragon avatar Mar 30 '23 19:03 edgarrmondragon

@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 avatar Mar 31 '23 05:03 aaronsteers

@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!

WillDaSilva avatar Mar 31 '23 12:03 WillDaSilva

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.

stale[bot] avatar Jul 29 '23 16:07 stale[bot]

Considering trying griffe to warn ourselves about breaking API changes, in CI

edgarrmondragon avatar Jul 29 '23 19:07 edgarrmondragon

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 avatar Jul 29 '23 19:07 WillDaSilva

@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.

edgarrmondragon avatar Jul 31 '23 19:07 edgarrmondragon

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

edgarrmondragon avatar Mar 05 '24 02:03 edgarrmondragon