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

Comment on internal interfaces #294

Closed

Conversation

moolitayer
Copy link
Collaborator

No description provided.

@moolitayer moolitayer force-pushed the warn_internal_interface branch from 2710cc5 to b015dfb Compare January 23, 2018 10:19
@abonas
Copy link
Member

abonas commented Jan 23, 2018

Why is there a need to place those comments? Wont it be hard to maintain them?

Copy link
Collaborator

@cben cben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abonas Having reviewed many PRs recently for "is this safe for 2.x or is it breaking change to API", I feel it's not clear enough to users what's considered "public API of kubeclient".

I don't know yet if this is best way to mark/document this. Suggestions very welcome...

@@ -18,6 +19,7 @@ def to_s
end
end

# Note: this IS NOT(!) a supported interface, and might change without a version bump
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? what are people supposed to do? they need to know what exceptions to expect from us.

@@ -3,6 +3,7 @@
module Kubeclient
module Common
# HTTP Stream used to watch changes on entities
# Note: this IS NOT(!) a supported interface, and might change without a version bump
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think only WatchStream.new is internal here. This is an object users get back from .watch_pods etc., and .watch and .finish are methods users interact with, both mentioned in README I think.

@moolitayer
Copy link
Collaborator Author

moolitayer commented Jan 25, 2018

WatchStream.initialize (as well as other initialize methods) really seem like the only relevant places. It seems users would have to be doing something out of the ordinary (and undocumented) to be using it.

I'd prefer to close this one

@moolitayer moolitayer force-pushed the warn_internal_interface branch from b015dfb to b08efd9 Compare January 25, 2018 14:08
@cben
Copy link
Collaborator

cben commented Jan 25, 2018

Thanks for the analysis. Agreed.

@moolitayer moolitayer closed this Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants