Skip to content

chore: use more fully-qualified names, so that an action can actually be called "Action"#579

Merged
jrudolph merged 3 commits intolightbend:mainfrom
jrudolph:fqn-names-in-codegen
Oct 7, 2021
Merged

chore: use more fully-qualified names, so that an action can actually be called "Action"#579
jrudolph merged 3 commits intolightbend:mainfrom
jrudolph:fqn-names-in-codegen

Conversation

@jrudolph
Copy link
Copy Markdown
Contributor

@jrudolph jrudolph commented Oct 7, 2021

The TCK service defined in the framework (https://github.com/lightbend/akkaserverless-framework/blob/0de0598276015466b4a7ae97e9d63052919869dd/protocols/tck/src/main/protobuf/akkaserverless/tck/model/action/action.proto) is called Action, so names clash if we want to generate stubs for it.

Action might be generic and short enough that users might also stumble over this. Using more fully-qualified names in code generation avoids this issue.

It's a common problem in code generation that you can get naming clashes. We could go all in and use only FQNs for our fixed symbols but that seems a bit of overkill. Changing it for something like Action on the other hand might be useful. WDYT?

@jrudolph jrudolph changed the title chore: use more FQN, so that an action can actually be called "Action" chore: use more fully-qualified names, so that an action can actually be called "Action" Oct 7, 2021
Copy link
Copy Markdown
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

I agree we should take this into account better (#437).

Using FQN's in more places would help. I agree with @patriknw that using FQN's everywhere, expecially in the 'unmanaged' templates, is not a great user experience. It looks like this PR only touches the abstract action and the handler, which are both managed, so that seems OK to me.

What I would like to do longer-term is solve this in the typeName and Imports utilities: we could detect clashes there, and add logic to use FQN's only when they're actually needed to avoid clashes. That needs some refactoring though. I'd be OK with merging this now to get you unstuck for the TCK, and the picking up the more 'minimal' approach later.

Copy link
Copy Markdown
Contributor

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

I agree on fq names in the managed classes, but would be very noisy to always have in the user/unmanaged classes by default IMO.

@jrudolph
Copy link
Copy Markdown
Contributor Author

jrudolph commented Oct 7, 2021

Yes, fully agree on keeping unmanaged generated sources clean. I'll have another try at decreasing the verbosity.

@jrudolph
Copy link
Copy Markdown
Contributor Author

jrudolph commented Oct 7, 2021

I only touch managed bits but I now made it a slight bit less verbose by importing Action.Effect directly instead of fqn'ing it. Of course, it's a bit arbitrary to allow usage of Action but not Effect but at least that will work for the TCK.

@jrudolph jrudolph merged commit fda7f1a into lightbend:main Oct 7, 2021
@jrudolph jrudolph deleted the fqn-names-in-codegen branch October 7, 2021 10:03
Copy link
Copy Markdown
Contributor

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants