meltano / sdk

Write 70% less code by using the SDK to build custom extractors and loaders that adhere to the Singer standard: https://sdk.meltano.com
https://sdk.meltano.com
Apache License 2.0
100 stars 70 forks source link

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

Closed aaronsteers closed 4 months ago

aaronsteers commented 1 year ago

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.
edgarrmondragon commented 1 year ago

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

aaronsteers commented 1 year ago

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

WillDaSilva commented 1 year ago

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

stale[bot] commented 1 year ago

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.

edgarrmondragon commented 1 year ago

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

WillDaSilva commented 1 year ago

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.

edgarrmondragon commented 1 year ago

@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 commented 8 months ago

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