SampleGen: default calling forms #2713
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #2713 +/- ##
============================================
+ Coverage 86.95% 86.96% +<.01%
- Complexity 5622 5645 +23
============================================
Files 468 469 +1
Lines 22446 22532 +86
Branches 2433 2437 +4
============================================
+ Hits 19519 19595 +76
- Misses 2079 2085 +6
- Partials 848 852 +4
Continue to review full report at Codecov.
|
|
PTAL @googleapis/samplegen |
|
@vchudnov-g We can discuss more on whether to let users specify |
| @@ -0,0 +1,40 @@ | |||
| /* Copyright 2018 Google LLC | |||
|
|
||
| public abstract SampleSpec.SampleType type(); | ||
|
|
||
| public static SampleConfig create( |
There was a problem hiding this comment.
Consider sticking with the Builder pattern for consistency.
| @@ -112,40 +114,76 @@ private static boolean expressionMatchesId(String expression, String id) { | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the SampleValueSets that were specified for this methodForm and sampleType. | |||
| * Returns all the `ids` that can match to one or more elements of `expressions` through regex. | |||
There was a problem hiding this comment.
Is this correct?: "Matches all the IDs within targets that match one or more elements of expressions. The IDs are extracted from elements of targets via targetToId."
| * sets, we use the default one. For incode samples, we always use the default value set derived | ||
| * from sample_code_init_fields for backward compatibility. | ||
| * | ||
| * <p>We should probably disable default value sets after incode samples stop using |
There was a problem hiding this comment.
IIUC, I agree. For all samples (standalone samples now, and in-code samples later), if no value set is specified, then no sample should be produced (this is the case already in standalone, right? That doesn't change?).
This is the only way to suppress samples with default calling forms: not specifying a value set.
There was a problem hiding this comment.
Re last paragraph: or not specify samples directive at all.
| * `defaultCallingForm` can match none of the calling forms in `allValidCallingForms`. For | ||
| * example, the `defaultCallingForm` for a C# unary call is `Request`, but it's not a valid | ||
| * calling form when generating asynchronous samples using the generated asynchonous public | ||
| * method. In this case we will not generate samples for this method. |
There was a problem hiding this comment.
To clarify: "Each RPC could result in multiple library methods. Not all these library methods will have default calling forms, but we intend for every RPC to have a default calling form in at least one of its library methods."
| regionTagCount.get(regionTag) != null && regionTagCount.get(regionTag) > 0, | ||
| "Sample not registered."); | ||
| String fileName; | ||
| if (regionTagCount.get(regionTag) == 1) { |
There was a problem hiding this comment.
Since this method and the previous share some code, consider whether factoring out the logic makes sense:
public ... GetSampleClassName(...) {
return constructName(sample, method, namer.getApiSampleClassName);
}
public ... GetSampleFileName(...) {
filename = constructName(sample, method, namer.getApiSampleFileName);
addFile(fileName, ...);
return fileName;
}
private ... constructName(MethodSampleView sample, String method, ... namingFunc) {
...
}
There was a problem hiding this comment.
Good catch! But I think passing the lambda here is a bit not Java idiomatic.
| @@ -792,10 +792,10 @@ public String getApiSampleClassName(String... parts) { | |||
| } | |||
|
|
|||
| /** | |||
| * The name of the file holding the sample class for a single API method and variant. The variant | |||
| * is typically a calling form. | |||
| * The name of the file holding the sample class for a single API method and variant. Constucted | |||
There was a problem hiding this comment.
nit: "The name is constructed...." (and sp on "constructed")
(It seems like in languages where the first part of the comment is a phrase, subsequent content is full sentences)
| @@ -25,13 +28,57 @@ | |||
| public class SampleFileRegistry { | |||
|
|
|||
| private final Map<String, SampleInfo> files = new HashMap<>(); | |||
| private final Map<String, Integer> regionTagCount = new HashMap<>(); | |||
There was a problem hiding this comment.
Could you expand the class comments just slightly to mention how the region tag affects the naming?
| @@ -181,6 +182,48 @@ static RpcType fromMethodContext(MethodContext context) { | |||
| .put(RUBY, RpcType.BIDI_STREAMING, ImmutableList.of(RequestStreamingBidi)) | |||
| .build(); | |||
|
|
|||
| private static final Table<TargetLanguage, RpcType, CallingForm> DEFAULT_CALLING_FORM_TABLE = | |||
| ImmutableTable.<TargetLanguage, RpcType, CallingForm>builder() | |||
There was a problem hiding this comment.
It'll be nice when we can read this from a config file..... ;-)
| sampleStreamBooks(); | ||
| } | ||
| } | ||
| // FIXME: Insert here clean-up code. |
There was a problem hiding this comment.
Do we still need this FIXME?
There was a problem hiding this comment.
No we don't! I'm gonna do it in a chaser PR, together with some other Java template fixes.
When:
```yaml
- samples:
standalone:
- region_tag: some_tag
# unspecified calling forms
value_sets: a_value_set, b_value_set
```
Default calling forms are used.
Also use region tags as file names when there is only one sample that has this region tag.
When:
Default calling forms are used.
Also use region tags as file names when there is only one sample that has this region tag.
Apologies that this PR is causing huge changes in the baselines :( This is unavoidable when we rename files.
To review the styles of default calling forms: Please search
"Test default calling forms for unary methods."(unary),"Test default calling form for paging methods."(paging) and"this_tag_should_be_the_name_of_the_file"(LRO).