Skip to content

Conversation

@youngjungithub
Copy link
Contributor

@youngjungithub youngjungithub commented Mar 14, 2025

Query parameters in the OpenAPI specifications are incorrectly defined as type: string, even when they represent other data types such as integers (e.g., count=100).

This misrepresentation causes confusion for API consumers, as the specs do not accurately reflect the expected data types. It also undermines proper validation and the quality of auto-generated documentation.

@youngjungithub youngjungithub requested a review from a team as a code owner March 14, 2025 16:00
@youngjungithub youngjungithub marked this pull request as draft March 14, 2025 16:00
@youngjungithub youngjungithub changed the title fix: openapi spec schmema.type fix: openapi spec schema.type Mar 14, 2025
@youngjungithub youngjungithub force-pushed the queryparam-rework branch 2 times, most recently from e37f6fa to 75b09a0 Compare March 17, 2025 18:43
@youngjungithub youngjungithub marked this pull request as ready for review March 18, 2025 14:32
@youngjungithub youngjungithub marked this pull request as draft March 18, 2025 14:35
@youngjungithub youngjungithub marked this pull request as ready for review March 18, 2025 14:37
@ericcrosson-bitgo ericcrosson-bitgo marked this pull request as draft March 20, 2025 16:33
@ericcrosson-bitgo
Copy link
Contributor

ericcrosson-bitgo commented Mar 20, 2025

@youngjungithub, I've put this back into draft until the following action items are complete:

  1. Please rebase on latest master -- I see merge conflicts locally stemming from prettier formatting
  2. Please combine all commits into one, as a way to improve the signal-to-noise ratio of the commit log
  3. Please improve the commit message, which you can then use as the PR body.
    Recall that our release pipeline uses your commit messages to derive release notes, so we should write commit messages to contain the information we want our release notes to contain. Here is an example of a good release note -- it includes a summary of the change, and the backing motivation; it helps the reader visualize which parts of an apiSpec will generate different OpenAPI output, and how that output has changed.

@youngjungithub youngjungithub force-pushed the queryparam-rework branch 2 times, most recently from a1c893c to a69b8e5 Compare March 20, 2025 17:40
@youngjungithub youngjungithub marked this pull request as ready for review March 20, 2025 17:43
Copy link
Contributor

@ericcrosson-bitgo ericcrosson-bitgo left a comment

Choose a reason for hiding this comment

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

Outstanding work! You turned this into a readable addition, which in turn will make our OpenAPI output more readable and idiomatic.

There are two outstanding points to address:

  1. Please ensure all new functionality (lines of code may act as a proxy) is under test. This will prevent accidental regression in the future.
  2. Please rework the commit message so it is usable as a release note. Here is an example of a good commit message -- it includes a summary of the change, and the backing motivation; it helps the reader visualize which parts of an apiSpec will generate different OpenAPI output, and how that output has changed.
    This data may not seem important to add in a commit message, since much of the information can be seen in the commit itself, but the release pipeline uses this commit message to create release notes, and this information will be valuable to openapi-generator users.

Once these points are addressed, this code looks ready to ship!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great work in this file, @youngjungithub! Will you please adding any test cases necessary to ensure every custom mapping is under test?

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