Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Moving util.dict_to_tuple_key() into only module that uses it. #429

Merged
merged 1 commit into from
Feb 23, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 20, 2016

When I filed #428 I compared the modules to see what was superfluous and noticed that this method only had one user. This change may not be "allowed" since it removes a public interface.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 20, 2016

For some reason this didn't trigger the Travis hook?

@@ -48,6 +48,24 @@ def filename(self):
return self.filename_str


class Test__dict_to_tuple_key(unittest.TestCase):

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 20, 2016

@nathanielmanistaatgoogle Don't forget to weigh in on whether or not moving this is allowed (since it is a public interface)

@nathanielmanistaatgoogle
Copy link
Contributor

I endorse this change, despite it technically regressing the public interface of the library. The code element being withdrawn has nothing to do with authentication or cryptography or security or anything else that is part of the intended surface area of the library.

I was actually just the other day defending another library's many many underscores and highly restricted public interface and I think this a good example of what I was describing in that conversation about how too often things are accidentally public because underscores are cosmetically unattractive and library authors are slow to adopt a habit of "everything hidden by default when first written and then public only after thoughtful consideration that it needs to be public to serve the library's users". :-)

@dhermes
Copy link
Contributor Author

dhermes commented Feb 20, 2016

I am a big fan of that approach.

I addressed the review comments. PTAL.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 20, 2016

And we even got Travis to fire on the new commit!

@nathanielmanistaatgoogle
Copy link
Contributor

Looks great; squash, wait a reasonable time for test results (it is Saturday after all), and merge. :-)

@dhermes dhermes force-pushed the move-dict-to-tuple branch 2 times, most recently from fd1cb60 to 5189cbd Compare February 23, 2016 01:06
dhermes added a commit that referenced this pull request Feb 23, 2016
Moving util.dict_to_tuple_key() into only module that uses it.
@dhermes dhermes merged commit 55ed32d into googleapis:master Feb 23, 2016
@dhermes dhermes deleted the move-dict-to-tuple branch February 23, 2016 05:53
@dhermes dhermes mentioned this pull request Mar 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants