fix: allow empty services and java keywords as a method names

[vam-google]

This is specifically needed to enable sqladmin API, which has the following issues:

1. Using import, reserved java keyword, as a method name: https://github.com/googleapis/googleapis/blob/master/google/cloud/sql/v1/cloud_sql_instances.proto#L109
2. Has an empty service (no methods whatsoevver) defined: https://github.com/googleapis/googleapis/blob/master/google/cloud/sql/v1/cloud_sql_instance_names.proto#L31

I plan to add a full integration tests with sqladmin (which will also test pure REGAPIC case) once this is pushed (need to integrate these changes first for technical reasons).

[blakeli0]

I think each Sample will end up as a Java file, instead of creating an empty sample, we should probably either check it before entering the method, or return null and handle it later.

[vam-google]

Compiling an empty file does not result in error in java, so I was under impression that empy sample is a valid case here (i.e. empty sample for empty service and no need to have special handling for nulls). On the other hand, empty service is a suspicious thing in API surface in a first place, so maybe the best way to fix it is for API team to stop publishing empty service definition.

[blakeli0]

Yes, I agree that empty service should probably be prohibited in the first place, but in case it happens, is an empty sample file more helpful to developers? or not generating samples at all more helpful? I think the former might cause more confusion unless we are required to generate samples for empty services. @eaball35 what's your take on this?

[eaball35]

I agree this shouldn't even be a case in the first place, but here we are. I think it would be the preferred developer experience to not generate the empty sample. It's not that big of a deal to generate it, but it won't provide much value and will be confusing. Checking before entering or return null/optional and handling makes sense

[vam-google]

@blakeli0, @eaball35 Ok, I changed it to generate a legit sample to the extent possible - it shows how to create a client, but does not call any methods on the generated client, since there are none. I think it is a good balance between possible options. This will produce a valid compilable sample - a pretty useless (but valid) example for a pretty useless (empty) service. The rest is on the API side, I think - once they add methods we would not have this problem.

[eaball35]

I like this better because it's clear this behavior is more intentional, but "empty" is kind of a confusing name.

The goal of generating these samples in files with region tags is so they can be more easily be pulled into documentation/sample browser as desired. I can't see why anyone would ever want these "empty" samples. It's not going to necessarily hurt to include them, but it doesn't provide any value. What is the pushback against just not generating them?

[vam-google]

Because we still generate the client class. The class still has methods, create is one of them, so the sample generates as much as it can for a specific client class and it is still a valid example. Moving nulls around is an antipattern in java, plus, if we stop generating the sample we will have to change accordingly the whole header of the generated class documentation, which currently says:

This class provides the ability to make remote calls to the backing service through method calls that map to API methods. Sample code to get started:

I.e. not generating sample at all will have impliaitons on this comment (we can't say "Sample codeto get started" if there is no sample code). Also, assuming Emily's samples are there in their final form: still generating sample would let that code not to have to bother with special null handling, it can simply assume that sample is always generated. I.e. it literally feels to me like a better, simpler and more robust design. Otherwise all sampling logic would always have to be null-aware (not the case now), which unnecessarily complicates things. Yes, the emply sample is basically useless. But it is a useless (but formally valid) sample for a useless (but formally valid) client class, so they are in a perfect harmony here...

On the other hand, this is a very minor thing. Mainly, empty services should not even be a thing. Here I'm just trying to make presence of an empty service in API not fail the generation of the client for the whole API (which has many other non-empty services). Basically the whole goal of this is to make generator stop failing if there is a bogus service and ensure some more or less reasonable behavior of generator in case like this, without imposing a burden on the rest of the team on maintaining nulls.

[eaball35]

Thanks for explaining. I was thinking maybe returning an Optional< Sample> for that method and handling but can see the annoyance still. I still think creating the empty sample is kind of weird, but I understand why you want to do this. Because this is such a minor case that will rarely appear I guess it's fine to do this way. I can always add logic to the Writer to ignore writing "empty" samples. Maybe add some additional comments on composeEmptySample to explain what it's for, otherwise lgtm.

[vam-google]

Using Optional<Sample> would break the whole contract in samples generator and make it inconsistent with the rest of the methods (having one method in ServiceClientMethodSampleComposer return Optional<Sample> while the rest return Sample would be weird).

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[vam-google]

@eaball35 Changed the name of the method and added some comments. PTAL

[blakeli0]

@eaball35 Changed the name of the method and added some comments. PTAL

Emily is out today but it looks good to me based the conversation history.