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

Adds support for conversion of API manifest document to OpenAI Plugin manifest #34

Merged
merged 11 commits into from
Oct 18, 2023

Conversation

peombwa
Copy link
Contributor

@peombwa peombwa commented Oct 12, 2023

This PR fixes #4 as a prerequisite for microsoft/OpenAPI.NET#1384 by:

  • Adds ToOpenAIPluginManifestAsync extension method to a valid instance of ApiManifestDocument. The method uses imported logic from Hidi to generate an OpenAI Plugin manifest.
  • Adds tests to validate the conversion.
  • Adds CHANGELOG.md.
  • Adds Mustafa to CODEOWNERS.

Open Questions:

  • Should we allow customers to set openApiFilePath parameter (optional) to ToOpenAIPluginManifestAsync method? openApiFilePath is required to instantiate OpenAI's Plugin manifest API property that points to an API specification (file path or URL). Should we instead default to ApiManifestDocument.ApiDescripionUrl property instead?
  • Should the library make web requests to fetch the OpenAPI document that's used to instantiate OpenApiManifest, or should we just expose ToOpenAIPluginManifest method that accepts an instance OpenApiDocument (customer fetches the OpenApiDocument outside the library)?

References

@peombwa peombwa self-assigned this Oct 13, 2023
@peombwa peombwa marked this pull request as ready for review October 13, 2023 17:20
@peombwa peombwa requested a review from zengin October 13, 2023 17:22
@peombwa peombwa changed the title Moves conversion of API manifest document to OpenAI Plugin manifest to an extension method Adds support for conversion of API manifest document to OpenAI Plugin manifest Oct 13, 2023
zengin
zengin previously requested changes Oct 13, 2023
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/TypeExtensions/ApiManifestDocumentExtensions.cs Outdated Show resolved Hide resolved
src/lib/TypeExtensions/ApiManifestDocumentExtensions.cs Outdated Show resolved Hide resolved
tests/ApiManifest.Tests/Helpers/ParsingHelpersTests.cs Outdated Show resolved Hide resolved
@zengin
Copy link

zengin commented Oct 13, 2023

@peombwa
For open questions:

  1. I think that the path represents something more general, it gives the impression that the file could also be local. If we are going to support scenarios like that, I think it might be a good idea to keep them separate. But if we are always expected to fetch from the ApiDescriptionUrl in all operations we expose, then not giving the option to specify path makes more sense to me.
  2. I think that we can expose both methods, thinking of a scenario where users of library already have an in-memory document object, not fetching could be a minor perf gain.

src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/TypeExtensions/ApiManifestDocumentExtensions.cs Outdated Show resolved Hide resolved
@peombwa peombwa requested review from baywet and zengin October 17, 2023 21:40
src/lib/Helpers/ParsingHelpers.cs Outdated Show resolved Hide resolved
src/lib/TypeExtensions/ApiManifestDocumentExtensions.cs Outdated Show resolved Hide resolved
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

@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

88.0% 88.0% Coverage
0.0% 0.0% Duplication

@peombwa peombwa dismissed zengin’s stale review October 18, 2023 18:57

Added code comment to helper method for clarity.

@peombwa peombwa merged commit cb11ed0 into main Oct 18, 2023
8 checks passed
@peombwa peombwa deleted the feature/GeneratePluginManifest branch October 18, 2023 18:57
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.

Move code to generate AIPluginManifest from hidi to this library
3 participants