Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for paths with alternate keys #308

Merged
merged 5 commits into from
Nov 23, 2022

Conversation

millicentachieng
Copy link
Member

@millicentachieng millicentachieng commented Nov 11, 2022

Fixes #120

This PR:

  • Adds new paths for entities that have alternate keys
  • Renames the summary and OperationId of operations to include key name
  • Add a setting to enable the generation of paths with alternate keys

@millicentachieng millicentachieng force-pushed the task/ma/add-alternate-key-paths branch from 576970e to a10262b Compare November 21, 2022 13:24
@millicentachieng millicentachieng force-pushed the task/ma/add-alternate-key-paths branch from a10262b to 400e122 Compare November 22, 2022 00:22
@millicentachieng millicentachieng marked this pull request as ready for review November 22, 2022 01:21
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

I also believe we should introduce a setting for that new behavior

@millicentachieng
Copy link
Member Author

I also believe we should introduce a setting for that new behavior

I had a discussion with @darrelmiller regarding this, and we thought that if there are alternate keys defined in the metadata then we ought to generate paths that utilize them as well.

Do you think the presence of these paths is going to affect any downstream tools? CC @peombwa

Co-authored-by: Vincent Biret <vibiret@microsoft.com>
@baywet
Copy link
Member

baywet commented Nov 22, 2022

It might increase significantly the size of the SDKs, especially for sets that are fairly "close to the root" of the API paths tree since the logic might go ahead and expand all the paths we already had under the main key.
Additionally, the "custom" has been to introduce settings as a way for consumers to control the behaviour of the conversion every time we've introduced changes that add additional paths to the resulting description. This way, should somebody complain about the new paths and not want them, they have a workaround.

@millicentachieng
Copy link
Member Author

@baywet We decided not to expand the paths using alternate keys. I will be adding the setting.

@baywet
Copy link
Member

baywet commented Nov 23, 2022

Thanks, that's what I suspected from the code but I wasn't sure whether it was a final decision or a stopgap.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

thanks for making the changes!

@peombwa
Copy link
Contributor

peombwa commented Nov 23, 2022

I also believe we should introduce a setting for that new behavior

I had a discussion with @darrelmiller regarding this, and we thought that if there are alternate keys defined in the metadata then we ought to generate paths that utilize them as well.

Do you think the presence of these paths is going to affect any downstream tools? CC @peombwa

As @baywet said above, we may see a significant growth in paths. However, having the setting to control how paths with alternate keys are generated is enough to warrant any concerns with downstream tools.

@millicentachieng millicentachieng merged commit 1989798 into master Nov 23, 2022
@millicentachieng millicentachieng deleted the task/ma/add-alternate-key-paths branch November 23, 2022 18:32
@millicentachieng millicentachieng self-assigned this Jan 24, 2023
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.

Add support for alternate keys
4 participants