Video Intelligence Documentation has VideoIntelligence listed twice

thagomizer commented 6 years ago

It should only be listed once.

blowmage commented 6 years ago

They are two different namespaces. One with "i", the other with "I".

quartzmo commented 6 years ago

This root issue of the namespaces was discussed in #1750, leading to googleapis/artman#288.

quartzmo commented 6 years ago

@geigerj Should we close this? Or assign it to you?

The latest docs for the gem at rubydoc.info still show the two namespaces.

geigerj commented 6 years ago

Sure, you can assign to me. This is basically pending the implementation of https://github.com/googleapis/artman/issues/288.

geigerj commented 6 years ago

Recap of this issue:

There are currently two very similar Video Intelligence modules (Google::Cloud::VideoIntelligence and Google::Cloud::Videointelligence -- differing on the casing of the i in intelligence), which should probably be merged.
The GAPIC code is generated into Google::Cloud::VideoIntelligence, whereas protoc-generated code is in Google::Cloud::Videointelligence
The GAPIC module is preferable, but the protoc module is not currently configurable
This sort of problem exists across GAPIC languages, and has been solved before, so Ruby autogen should be able to use an existing solution as a reference.
Some other languages have solved this by adding syntax for configuring the namespace of protoc output in proto syntax as an proto option (example for C#/Java/Go/PHP).
Python has addressed this problem with additional code generation: artman modifies the protoc output (see here), and toolkit exports the protoc-generated types into a predictable namespace (example here)

For Ruby, I do not recommend the proto option approach on the basis of PHP's recent experience implenting their proto extension. This is a heavyweight solution that requires substantial cost both to modify protoc and to manage a backfill of existing protos without breaking internal usage. I would probably recommend following a solution similar to Python's, which is what is tracked by googleapis/artman#288. @lukesneeringer and @michaelbausor are good folks to chat to for context on these two different approaches.

quartzmo commented 6 years ago

@jbolinger Is there any hope for resolution of this issue? It is now out of SLO.

jbolinger commented 6 years ago

@quartzmo there's some good news here. I requested a ruby_package protoc option awhile back and it was implemented (details on googleapis/artman#288). However, I'm not sure if it has been released and we haven't tried to use it yet (the docs don't mention the new option yet either).

We could ask for the protos to be updated to use the option, which should result in a single namespace. It will clearly be a breaking change. Do you know who to ping about doing that?

quartzmo commented 6 years ago

@jbolinger Congratulations on the progress with artman! Regarding the breaking change, the package is 1.0.0. I don't know who to ask about it.

blowmage commented 6 years ago

Perhaps identifying an internal owner is something @JustinBeckwith can help with. The code was first added by @swcloud in #1490.

A breaking change would require the library to go to 2.0, but I don't see that as being a problem.

theacodes commented 6 years ago

Could we possibly make the old (bad) namespace an alias for the new, fixed namespace? We'd also need to exclude the old namespace from docs.

theacodes commented 6 years ago

but I don't see that as being a problem.

This is actually a problem.

blowmage commented 6 years ago

Could we possibly make the old (bad) namespace an alias for the new, fixed namespace?

We proposed this when the issue first came up, but were told that because this gem is 100% generated any changes would need to come from the generated code. This is why the issue has remained open for so long that it is out of SLO.

blowmage commented 6 years ago

This is actually a problem.

Why is this a problem? The gem version doesn't track the API version. Why not release a 2.0 gem?

theacodes commented 6 years ago

That's silly. The whole point of our approach to client libraries is that we can make modifications. If you'll send a PR to add the alias, I'll codify it into a synthesis script for this package. We're experimenting with synthesis scripts in other languages specifically for reasons such as this.

Why is this a problem? The gem version doesn't track the API version. Why not release a 2.0 gem?

Because ostensibly we have to support every major version for at least a year.

blowmage commented 6 years ago

So you are proposing the following:

1) Generate new ruby files from the protos that use the new Google::Cloud::VideoIntelligence namespace instead of Google::Cloud::Videointelligence. 2) I'll add a pr that aliases Google::Cloud::VideoIntelligence as Google::Cloud::Videointelligence to maintain backwards compatibility. 3) You'll update the generator to incorporate the alias changes. 4) We generate new ruby files that include the alias changes.

Is that correct?

theacodes commented 6 years ago

Basically. Except we won't actually merge #1 or #2, I'll incorporate those into a single script and do a full regeneration using that.

blowmage commented 6 years ago

@jbolinger Can you open a PR that updates these files using the new namespace? I'll add the alias to maintain backwards compatibility to your PR.

jbolinger commented 6 years ago

@blowmage Sure, but we have to wait for the 3.6.0 protobuf release to land so we can use the new flag (shouldn't be long).

We've also merged the changes for credentials.rb that we discussed a few weeks ago into the generator so this might be a good time to develop a similar script to retain compatibility for it.

@theacodes who should make the proto changes? Can we just do it or should we request it be done by the backend team?

theacodes commented 6 years ago

@jbolinger is it just to change the protobuf package name for ruby? We can send a CL.

jbolinger commented 6 years ago

@theacodes Great - thanks for confirming. Yes, it's just a one line option like most of the other languages already have.

blowmage commented 6 years ago

We've also merged the changes for credentials.rb that we discussed a few weeks ago into the generator so this might be a good time to develop a similar script to retain compatibility for it.

Absolutely, I'll also make those changes that maintain backwards compatibility in your PR.

blowmage commented 6 years ago

@jbolinger Is this something we can start doing now?

jbolinger commented 6 years ago

@blowmage yup, we'll need to update the protos to get started. Either @dazuma or I can do that part pretty easily. Do we want to do this for all 4 versions of the API (I suspect yes)?

dazuma commented 6 years ago

I'd actually prefer to wait until after next week to get started on this, as we're going to need to coordinate some synth script changes to alias the old names, and I'm slammed for the next week due to ElixirConf. Unless someone else would like to tackle the synth script changes?

blowmage commented 5 years ago

It has been a few weeks. Are we ready to start on this? If not, when do we expect we can make this change?

blowmage commented 5 years ago

We are looking into setting ruby_namespace in the protobuf files to resolve this.

blowmage commented 5 years ago

We have requested ruby_namespace to be set in the proto files, but haven't seen the change made yet. We hope this will happen in the next couple weeks.

sduskis commented 5 years ago

@blowmage and @dazuma, is this fixed by #3582?

blowmage commented 5 years ago

Yes, closing.

googleapis / google-cloud-ruby

Video Intelligence Documentation has VideoIntelligence listed twice #1762