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

Merge util.py and _helpers.py #579

Merged
merged 1 commit into from Aug 5, 2016

Conversation

Projects
None yet
5 participants
@pferate
Contributor

pferate commented Aug 1, 2016

A new file, _helpers.py, was created without realizing that utils.py existed for the same purpose.

Resolves: #251, (partially) #428 (partial duplicate of #251)

@googlebot googlebot added the cla: yes label Aug 1, 2016

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 1, 2016

Contributor

@pferate I am the originator of _helpers and it was very much intentional. If you are merging them then utils should go away since it is public.

Also, I seem to recall there being an issue with the LICENSE on that file since it came from Guido on the protorpc project

Contributor

dhermes commented Aug 1, 2016

@pferate I am the originator of _helpers and it was very much intentional. If you are merging them then utils should go away since it is public.

Also, I seem to recall there being an issue with the LICENSE on that file since it came from Guido on the protorpc project

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 1, 2016

Contributor

Thanks for the info. I guess I misunderstood your comment from #251:

That's my fault for creating _helpers and not realizing util was there.

and assumed that the code in _helpers should go into utils.

Contributor

pferate commented Aug 1, 2016

Thanks for the info. I guess I misunderstood your comment from #251:

That's my fault for creating _helpers and not realizing util was there.

and assumed that the code in _helpers should go into utils.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 1, 2016

Contributor

Ahh that's a failure of my memory then, the thing I wrote at that time is the real truth.

The goal is a smaller surface area, not a larger one and none of the functions in _helpers are in the API surface area, so big 👍 there.

In addition to nuking util.py, I'd love to see a plan for deprecating use of the positional decorator.

Contributor

dhermes commented Aug 1, 2016

Ahh that's a failure of my memory then, the thing I wrote at that time is the real truth.

The goal is a smaller surface area, not a larger one and none of the functions in _helpers are in the API surface area, so big 👍 there.

In addition to nuking util.py, I'd love to see a plan for deprecating use of the positional decorator.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 1, 2016

Member

In addition to nuking util.py, I'd love to see a plan for deprecating use of the positional decorator.

File a bug and put it in the 4.0.0 milestone.

Member

theacodes commented Aug 1, 2016

In addition to nuking util.py, I'd love to see a plan for deprecating use of the positional decorator.

File a bug and put it in the 4.0.0 milestone.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 1, 2016

Member

In general, I'd prefer less public stuff in this library.

Member

theacodes commented Aug 1, 2016

In general, I'd prefer less public stuff in this library.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 1, 2016

Contributor

File a bug and put it in the 4.0.0 milestone.

I apparently already filed #428 for just this and you and @pferate got it in the right milestone

Contributor

dhermes commented Aug 1, 2016

File a bug and put it in the 4.0.0 milestone.

I apparently already filed #428 for just this and you and @pferate got it in the right milestone

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 2, 2016

Contributor

... so is this pull request actionable or should it be closed? Is this where util.py should be killed off?

Contributor

nathanielmanistaatgoogle commented Aug 2, 2016

... so is this pull request actionable or should it be closed? Is this where util.py should be killed off?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 3, 2016

Member

I don't see anything in utils that want to be public. Should merge utils into _helpers and remove utils?

Member

theacodes commented Aug 3, 2016

I don't see anything in utils that want to be public. Should merge utils into _helpers and remove utils?

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 3, 2016

Contributor

Yes that

Contributor

dhermes commented Aug 3, 2016

Yes that

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 3, 2016

Contributor

OK, I'll update this PR. With _helpers being private, should all of the functions and global variables within it be private as well? Or would that already be handled from the private module?

Contributor

pferate commented Aug 3, 2016

OK, I'll update this PR. With _helpers being private, should all of the functions and global variables within it be private as well? Or would that already be handled from the private module?

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 3, 2016

Contributor

That is @nathanielmanistaatgoogle's territory (of having more refined opinions than others). IIUC he prefers that imported functions be public, and since the module is non-public, that will be very much OK. (I would suggest holding off of renames like _helpers._from_bytes for a separate PR.)

Contributor

dhermes commented Aug 3, 2016

That is @nathanielmanistaatgoogle's territory (of having more refined opinions than others). IIUC he prefers that imported functions be public, and since the module is non-public, that will be very much OK. (I would suggest holding off of renames like _helpers._from_bytes for a separate PR.)

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 3, 2016

Member

I also would prefer _helpers.from_bytes instead of _helpers._from_bytes.

Basically, functions that start with _ are module-private and modules that start with _ are package-private. A package-private module still has a right to have module-private attributes.

Member

theacodes commented Aug 3, 2016

I also would prefer _helpers.from_bytes instead of _helpers._from_bytes.

Basically, functions that start with _ are module-private and modules that start with _ are package-private. A package-private module still has a right to have module-private attributes.

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 3, 2016

Contributor

Thanks!

Contributor

pferate commented Aug 3, 2016

Thanks!

@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 4, 2016

Contributor

PR has been updated (utils -> _helpers).

Contributor

pferate commented Aug 4, 2016

PR has been updated (utils -> _helpers).

return scopes
def _add_query_parameter(url, name, value):

This comment has been minimized.

@theacodes

theacodes Aug 4, 2016

Member

want to go ahead and remove the _ here?

@theacodes

theacodes Aug 4, 2016

Member

want to go ahead and remove the _ here?

This comment has been minimized.

@pferate

pferate Aug 4, 2016

Contributor

@dhermes suggested to remove the leading _'s in a separate PR. Or is this one different since I'm already touching the function?

@pferate

pferate Aug 4, 2016

Contributor

@dhermes suggested to remove the leading _'s in a separate PR. Or is this one different since I'm already touching the function?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 4, 2016

Member

This change looks mostly good to me, @nathanielmanistaatgoogle to do the final review.

Member

theacodes commented Aug 4, 2016

This change looks mostly good to me, @nathanielmanistaatgoogle to do the final review.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle Aug 4, 2016

Contributor

Done with my first round of review.

Contributor

nathanielmanistaatgoogle commented Aug 4, 2016

Done with my first round of review.

Show outdated Hide outdated tests/test__helpers.py Outdated
Merge util.py and _helpers.py
A new file, `_helpers.py`, was created without realizing that
`utils.py` existed for the same purpose.

Moving all to `_helpers.py`.
@pferate

This comment has been minimized.

Show comment
Hide comment
@pferate

pferate Aug 4, 2016

Contributor

PR has been updated. I will be creating another PR to remove the _ from functions in _helpers.

Contributor

pferate commented Aug 4, 2016

PR has been updated. I will be creating another PR to remove the _ from functions in _helpers.

@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit 248dc6c into googleapis:master Aug 5, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@pferate pferate deleted the pferate:refactor_helpers branch Aug 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment