Skip to content
This repository was archived by the owner on Jun 16, 2026. It is now read-only.

Honor x-accessibility on operations#1145

Merged
chamons merged 11 commits into
Azure:feature/v3from
chamons:llc_access
Apr 13, 2021
Merged

Honor x-accessibility on operations#1145
chamons merged 11 commits into
Azure:feature/v3from
chamons:llc_access

Conversation

@chamons

@chamons chamons commented Apr 12, 2021

Copy link
Copy Markdown
Contributor

@chamons
Honor x-accessibility on operations
67d3dcb 
- Fixes: Azure#1134
- Add LLC variant of TestProjects tests
@chamons
chamons requested review from m-nash, pakrym and weshaggard April 12, 2021 15:27
@chamons
chamons requested a review from ShivangiReja as a code owner April 12, 2021 15:27
@chamons

chamons commented Apr 12, 2021

Copy link
Copy Markdown
Contributor Author

Here is the current PR diff with no generated files - https://gist.github.com/chamons/561ebeb0f34bfc1424b3204c746d258b

pakrym
pakrym reviewed Apr 12, 2021
View reviewed changes
Comment thread eng/Generate.ps1 Outdated
pakrym
pakrym reviewed Apr 12, 2021
View reviewed changes
Comment thread src/AutoRest.CSharp/Common/Generation/Writers/RequestWriterHelpers.cs Outdated
@pakrym

pakrym commented Apr 12, 2021

Copy link
Copy Markdown
Contributor

Can you please add a section to the README.md about this feature?

@chamons

chamons commented Apr 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Question @pakrym

This PR previously didn't touch RestClientWriter because I don't have test coverage (or even know if this should apply there).

I see a few options:

  • 1 - Keep the ClientMethod / RestClientMethod model to nullable and set it to null everywhere i'm not explicitly testing.
    • Advantage: I'm not mucking with code I don't have good test coverage for. Keep PR small.
    • Disadvantage: Some writers are hard coding public in places that may need to honor x-accessibility. If I miss a place we might have a null floating around.
  • 2 - Insist that all writers stop hard coding method visibility, as that should be a model concern.
    • Advantage: Architectural purity
    • Disadvantage: Either I write a bunch more tests or I'm changing behavior with no visibility.

I'm strongly leaning towards #1, but I'm not sure if that is laziness due to lack of second cup of coffee or being smart.

@pakrym

pakrym commented Apr 12, 2021

Copy link
Copy Markdown
Contributor

I'll go with # 2 and see if it changes any generated code. It should be pretty obvious if you've broken anything.

@chamons
chamons marked this pull request as draft April 12, 2021 16:36
@chamons

chamons commented Apr 12, 2021

Copy link
Copy Markdown
Contributor Author

Moving to draft given size of changes likely to go in.

pakrym
pakrym reviewed Apr 12, 2021
View reviewed changes
Comment thread src/AutoRest.CSharp/LowLevel/Output/LowLevelRestClient.cs Outdated

Request request = new Request (method.Request.HttpMethod, method.Request.PathSegments, method.Request.Query, method.Request.Headers, body);
yield return new RestClientMethod (method.Name, method.Description, method.ReturnType, request, parameters.ToArray(), method.Responses, method.HeaderModel, method.BufferResponse, method.Accessibility);
yield return new RestClientMethod (method.Name, method.Description, method.ReturnType, request, parameters.ToArray(), method.Responses, method.HeaderModel, method.BufferResponse, accessibility);

@pakrym pakrym Apr 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pull it off method.BufferResponse?

@chamons chamons Apr 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you are asking here?

@pakrym pakrym Apr 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry. You seem to be passing accessibility both into BuildMethod and here. Why both?

@chamons chamons Apr 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're calculating the "base" method via _builder.BuildMethod and then clone it with modifications.

I think the way I'm doing it is technically correct, but just copying it over with:

yield return new RestClientMethod (method.Name, method.Description, method.ReturnType, request, parameters.ToArray(), method.Responses, method.HeaderModel, method.BufferResponse, method.Accessibility);

would be more clear.

@chamons

chamons commented Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

Test failure a known issue (that i'm going to look at next) - #1152

@chamons
chamons marked this pull request as ready for review April 13, 2021 17:35
@chamons
chamons merged commit 7740e3c into Azure:feature/v3 Apr 13, 2021
@chamons
chamons deleted the llc_access branch April 13, 2021 18:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weshaggard weshaggard Awaiting requested review from weshaggard
@m-nash m-nash Awaiting requested review from m-nash
@ShivangiReja ShivangiReja Awaiting requested review from ShivangiReja
1 more reviewer
@pakrym pakrym pakrym approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[LLC] Add support for x-accessibility

2 participants

@chamons @pakrym