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.

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

Use jaraco.classes for properties #588

Merged
merged 2 commits into from Sep 1, 2022
Merged

Use jaraco.classes for properties #588

merged 2 commits into from Sep 1, 2022

Conversation

jaraco
Copy link
Owner

@jaraco jaraco commented Aug 8, 2022

Today I was reviewing the implementation of NonDataProperty and ClassProperty only to find that there were stale versions of the implementations in keyring.util.properties that represent old versions of the original implementations in jaraco.classes.

I'm tempted to simply depend on that code. Because it introduces a new dependency, I'm somewhat reluctant to accept the change. I'm putting it here for feedback and consideration.

@Darsstar
Copy link

Darsstar commented Aug 9, 2022

Maybe reexport them for the time being for backward compatibility? (I'm tempted to replace "Maybe" with "At least", but I feel that makes me sounds too entitled.)

@jaraco
Copy link
Owner Author

jaraco commented Sep 1, 2022

Maybe reexport them for the time being for backward compatibility? (I'm tempted to replace "Maybe" with "At least", but I feel that makes me sounds too entitled.)

I can do that - make it an optional dependency for the time being.

@jaraco jaraco merged commit 977ed03 into main Sep 1, 2022
@jaraco jaraco deleted the feature/jaraco.classes branch September 1, 2022 12:33
@Thaodan
Copy link

Thaodan commented Sep 8, 2022

Isn't that kinda much to add another dependency just for these two methods?

@jaraco
Copy link
Owner Author

jaraco commented Sep 8, 2022

Yes and no.

  1. In an ideal world, dependencies are free. And for most users, that's also true (pip install keyring just works in all cases). Of course, there are practical, non-ideal considerations.
  2. There are several costs to limiting dependencies. It increases friction when creating new functionality. It encourages vendoring (maintaining a local fork) of the functionality, which has its own maintenance costs and risks. It discourages best practices around dependency management.
  3. Not using the dependency masks the true dependency. It hides the fact that this functionality is useful across multiple projects, making it nearly impossible to determine if this shared need would be valuable to be exposed elsewhere (stdlib, for example).
  4. This case is a perfect case for including the dependency. There was an old, stale implementation that was copied from many years ago. Since then, the implementation has improved and evolved. In the meantime, other libraries developed a dependency on this vendored fork of the code.

Principles I roughly follow for dependencies:

  • If a small (~10 lines or less) piece of code is useful, it's okay to copy it, but still link to the source from which it was copied.
  • Consider using a dependency when the code is larger, there is more than one function, or the behavior is likely to evolve.
  • Take extra precautions when adopting the first dependency, as the distance from 0 to 1 is much greater than adding one more.

I think implicit in your question is that adding a dependency is a cost that should be taken seriously. Can you articulate or link to a resource that explains those costs?

@yan12125
Copy link

try:
    from jaraco.compat import properties  # pragma: no-cover
except ImportError:
    from . import _properties_compat as properties  # pragma: no-cover

I think from jaraco.compat import properties should be from jaraco.classes import properties?

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.

None yet

4 participants