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

Update imports to only Packages or Modules #562

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

pferate
Copy link
Contributor

@pferate pferate commented Jul 21, 2016

Also cleaned up some nested attribute access.

Resolves issue #556

@pferate
Copy link
Contributor Author

pferate commented Jul 22, 2016

I got all but one line down to under 80 characters. Without assigning temporary variables to shorten the line, it will take some refactoring beyond the scope of this PR to clean it up.

I've posted some options in #556 on how to proceed.

@dhermes
Copy link
Contributor

dhermes commented Jul 22, 2016

Just assign a temporary variable. Anything else isn't worth the headache

@pferate
Copy link
Contributor Author

pferate commented Jul 22, 2016

Will do.

I want to add a TODO to fix that, but need to reference something. I'm thinking of adding an issue to refactor the built-in credentials. It started as a small refactor in my head, but it's becoming bigger as I think about it more.

@pferate
Copy link
Contributor Author

pferate commented Jul 22, 2016

Rebased branch, added issue #564, and updated this PR.

@theacodes
Copy link
Contributor

@pferate this branch has conflicts, can you update?

@pferate
Copy link
Contributor Author

pferate commented Jul 25, 2016

@jonparrott: Updated


urlpatterns += [url(r'^oauth2/', include(oauth2_urls))]
urlpatterns += [url(r'^oauth2/', include(site.oauth2_urls))]

This comment was marked as spam.

@@ -56,6 +49,8 @@

logger = logging.getLogger(__name__)

Credentials = client.Credentials

This comment was marked as spam.

This comment was marked as spam.

@nathanielmanistaatgoogle
Copy link
Contributor

Oof, that's a long code review. I keep forgetting that we have this much code in the library.

Looks mostly good; just those few observations.

Also cleaned up some nested attribute access.
@pferate
Copy link
Contributor Author

pferate commented Jul 27, 2016

Thanks @nathanielmanistaatgoogle!

I've updated the PR, per your suggestions and rebased again.

I mentioned this in another PR, but the flake8 test is failing due to a bug in flake8-import-order (0.9 released yesterday). I filed an issue, so hopefully it will be fixed shortly.

@nathanielmanistaatgoogle
Copy link
Contributor

Link to the flake8 bug you filed?

@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit bb2386e into googleapis:master Jul 27, 2016
@pferate
Copy link
Contributor Author

pferate commented Jul 27, 2016

@pferate
Copy link
Contributor Author

pferate commented Jul 27, 2016

The flake8 bug has been resolved and released in 0.9.1.

@theacodes theacodes mentioned this pull request Jul 28, 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

5 participants