Skip to content

Comments

Multiple url parameters#2737

Merged
chunyu3 merged 3 commits intoAzure:feature/v3from
chunyu3:multipleUrlParameters
Sep 28, 2022
Merged

Multiple url parameters#2737
chunyu3 merged 3 commits intoAzure:feature/v3from
chunyu3:multipleUrlParameters

Conversation

@chunyu3
Copy link
Member

@chunyu3 chunyu3 commented Sep 23, 2022

Description

Fix #2730

  • support url which contains more than one url parameter

we need to support following Url schemas:

  1. http://{urlstr}
  2. http://{urlstr}/parameter/{parameterName}
  3. http://{namespace}.service.com
  4. http://{namespace}.service.com/parameter/{parameterName}

For #1, #2, we still change urlStr to endpoint with Url type and isEndpoint=true. And parameterName is a normal client parameter with String type, isEndpoint=false
For #3, #4, both namespace and parameterName are normal client parameter with string type, and isEndpoint=false

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first

@mpodwysocki
Copy link

@chunyu3 When trying to compile the C# CADL using this repo for Azure Notification Hubs, I get the following:

> azure-notificationhubs-cadl@1.0.0 build:csharp
> cadl compile main.cadl --emit ../autorest.csharp.fork/src/CADL.Extension/Emitter.Csharp/dist/src/index.js --output-path cadl-output/csharp

Cadl compiler v0.35.0

No API-Version provided.
Compilation completed successfully, output files are in ~/git/azure-notificationhubs-cadl/cadl-output/csharp.

This seems new where before it was picking up the API version successfully. See the current cadl.json where it picks up the API version successfully.

@chunyu3
Copy link
Member Author

chunyu3 commented Sep 26, 2022

 main.cadl --emit ../autorest.csharp.fork/src/CADL.Extension/Emitter.Csharp/dist/src/index.js --output-path cadl-output/csharp

I tried locally, but I cannot reproduce this issue. @mpodwysocki would you please try again? if it still has problem, we can setup meeting and work together for this, thanks.

@mpodwysocki
Copy link

@chunyu3 Ok, rechecked and all works as expected. Thanks

@chunyu3
Copy link
Member Author

chunyu3 commented Sep 26, 2022

@chunyu3 When trying to compile the C# CADL using this repo for Azure Notification Hubs, I get the following:

> azure-notificationhubs-cadl@1.0.0 build:csharp
> cadl compile main.cadl --emit ../autorest.csharp.fork/src/CADL.Extension/Emitter.Csharp/dist/src/index.js --output-path cadl-output/csharp

Cadl compiler v0.35.0

No API-Version provided.
Compilation completed successfully, output files are in ~/git/azure-notificationhubs-cadl/cadl-output/csharp.

This seems new where before it was picking up the API version successfully. See the current cadl.json where it picks up the API version successfully.

confirmed. This issue does not occur. close it.

@chunyu3 chunyu3 requested review from archerzz and pshao25 September 26, 2022 01:59
@pshao25
Copy link
Member

pshao25 commented Sep 26, 2022

What is the difference between #1 and #3?

In #2, is parameterName a client parameter?

@chunyu3
Copy link
Member Author

chunyu3 commented Sep 26, 2022

What is the difference between #1 and #3?

In #2, is parameterName a client parameter?

#1 the parameter urlstr is the whole endpoint, #3, the parameter is just part of the endpoint.
in all scenarios, the parameterName is parameter of url, it is client parameter.

// continue;
// }

const isEndpoint: boolean = endpoint === (`{${name}}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably can be even more specific and check for name "$host", similar to M4: https://github.com/Azure/autorest/blob/5149dd0311cd5fad437ac18cff1e387b0835c1a5/packages/extensions/modelerfour/src/modeler/modelerfour.ts#L1748

Hello @AlexanderSher cadl will not define host parameter as global client parameter, it only define @server to define url. So we may not need to check this.

@chunyu3 chunyu3 merged commit 169a477 into Azure:feature/v3 Sep 28, 2022
@chunyu3 chunyu3 deleted the multipleUrlParameters branch March 2, 2023 05:20
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.

C# CADL Generation issue

4 participants