Skip to content

Conversation

@jdisanti
Copy link
Contributor

@jdisanti jdisanti commented Aug 10, 2021

Motivation and Context

The UnionGenerator was generated functions that returned a Result that wasn't fully qualified, and this would fail if the service model also had a structure in it named Result (as is the case with transcribestreaming).

Description

This PR fixes the name collision and adds a naming obstacle course test for it.

Testing

  • Added a new naming obstacle course.

Checklist

  • I have updated the CHANGELOG

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -0,0 +1,53 @@
$version: "1.0"
namespace naming_obs_structs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to put this test into the existing naming-obstacle-course.smithy, but that one already defines operation Result, which would conflict unless it was in a different namespace.

I'm open to ideas here since I dislike the names here (ops vs structs) as they don't really make that much sense. Can we have a larger multi-namespace, multi-file, naming-obstacle-course service that still checks things such as the namespace being crate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not really, one service is one namespace. We could use renames but that would of course, defeat the purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can have nested namespaces in Smithy, so there's a chance this might work. No idea how our codegen handles it though.

@jdisanti jdisanti requested a review from rcoh August 11, 2021 00:01
@jdisanti jdisanti merged commit 58d0da5 into smithy-lang:main Aug 12, 2021
@jdisanti jdisanti deleted the fix-union-fn-name-collision branch August 31, 2021 23:23
@ysaito1001 ysaito1001 mentioned this pull request Oct 25, 2022
1 task
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.

2 participants