Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Python split imports into local and shared sets #2063

Merged
merged 15 commits into from
Jun 21, 2018
Merged

Python split imports into local and shared sets #2063

merged 15 commits into from
Jun 21, 2018

Conversation

andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Jun 12, 2018 •
edited
Loading

Resolves #2058

Using this PR to regen python bigquerydatatransfer client results in googleapis/google-cloud-python#5478

@andreamlin andreamlin force-pushed the python_local_shared_imports branch from 4a8efe8 to 456fb83 Compare June 12, 2018 20:36
@codecov-io
Copy link

codecov-io commented Jun 12, 2018 •
edited
Loading

Codecov Report

Merging #2063 into master will increase coverage by 0.02%.
The diff coverage is 96.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2063      +/-   ##
============================================
+ Coverage     86.83%   86.85%   +0.02%     
- Complexity     4987     4989       +2     
============================================
  Files           448      448              
  Lines         19722    19739      +17     
  Branches       2080     2083       +3     
============================================
+ Hits          17125    17144      +19     
+ Misses         1889     1886       -3     
- Partials        708      709       +1
Impacted Files Coverage Δ Complexity Δ
...oogle/api/codegen/viewmodel/ImportSectionView.java 100% <ø> (ø) 2 <0> (ø) ⬇️
...transformer/py/PythonImportSectionTransformer.java 98.25% <96.66%> (+1.08%) 62 <4> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1c4651...b0477d5. Read the comment docs.

tseaver
tseaver reviewed Jun 13, 2018
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

I'm not really familiar with the codebase, but the changes here do seem to address the requirement expressed in #2058.

@andreamlin andreamlin force-pushed the python_local_shared_imports branch from 456fb83 to 390f0cf Compare June 15, 2018 23:29
@andreamlin andreamlin requested a review from landrito June 15, 2018 23:29
@andreamlin andreamlin force-pushed the python_local_shared_imports branch from 390f0cf to 1271a05 Compare June 15, 2018 23:30
@andreamlin
Copy link
Contributor Author

PTAL

@andreamlin andreamlin requested a review from pongad June 15, 2018 23:30
pongad
pongad reviewed Jun 18, 2018
View reviewed changes
src/main/java/com/google/api/codegen/transformer/py/PythonImportSectionTransformer.java Outdated
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;

/* Creates the import sections of GAPIC generated code for Python. */
public class PythonImportSectionTransformer implements ImportSectionTransformer {

private enum IMPORT_TYPE {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/main/java/com/google/api/codegen/transformer/py/PythonImportSectionTransformer.java Outdated
Map<String, IMPORT_TYPE> importNamesAndTypes = new HashMap<>();

List<ProtoElement> elements = Lists.newArrayList(model.getRoots());
String serviceFullName = elements.get(0).getFullName();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/main/java/com/google/api/codegen/transformer/py/PythonImportSectionTransformer.java Outdated
}
return ImmutableList.<ImportFileView>builder().addAll(imports).build();
importView.localImports(ImmutableList.<ImportFileView>builder().addAll(localImports).build());

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/main/java/com/google/api/codegen/transformer/py/PythonImportSectionTransformer.java Outdated
appImports.add(generateAppImport(importFullName, entry.getValue().getNickname()));
switch (importNamesAndTypes.get(importFullName)) {
case LOCAL:
localImports.add(generateAppImport(importFullName, entry.getValue().getNickname()));

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

landrito
landrito reviewed Jun 18, 2018
View reviewed changes
Copy link
Contributor

@landrito landrito left a comment

Choose a reason for hiding this comment

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

Also, we have been slowly getting around to refactoring python snippet files to make it such that there are no extraneous function calls.

While we are in py/types.snip can you remove the @private body and @private header methods moving it's contents into the @snippet generate method?

src/main/java/com/google/api/codegen/viewmodel/ImportSectionView.java Outdated
public abstract List<ImportFileView> appImports();

/** Used in Python. */

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@andreamlin andreamlin changed the title Python split imports into local and shared sets (WIP) Python split imports into local and shared sets Jun 19, 2018
@andreamlin andreamlin force-pushed the python_local_shared_imports branch from 3465fe3 to cdf8057 Compare June 19, 2018 20:08
@andreamlin
Copy link
Contributor Author

PTAL

@andreamlin andreamlin force-pushed the python_local_shared_imports branch from 6c23e88 to 903d2f5 Compare June 19, 2018 23:12
@pongad
Copy link
Contributor

pongad commented Jun 21, 2018

LGTM but perhaps Python folks to sign off.

landrito
landrito approved these changes Jun 21, 2018
View reviewed changes
Copy link
Contributor

@landrito landrito left a comment

Choose a reason for hiding this comment

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

LGTM

@andreamlin andreamlin merged commit 3717434 into googleapis:master Jun 21, 2018
@andreamlin andreamlin deleted the python_local_shared_imports branch June 21, 2018 19:34
@tseaver tseaver mentioned this pull request Jun 21, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tseaver tseaver tseaver left review comments

@pongad pongad pongad left review comments

@landrito landrito landrito approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andreamlin @codecov-io @pongad @tseaver @landrito