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

feat: introduce ordered maps into libopenapi #205

Merged
merged 61 commits into from
Dec 15, 2023

Conversation

TristanSpeakEasy
Copy link
Contributor

@TristanSpeakEasy TristanSpeakEasy commented Nov 27, 2023

Picks up from #138 and tries to address #119

The main goals will be:

  • Support iterating through the high model in the same order as the OpenAPI document
  • Render documents out deterministically
  • Render documents out matching the original order (if just making light modifications to the document)
  • Control the order of the rendered out docs, for example when the order of path items or components are specifically reordered
  • Any maps stored within example or examples fields in schemas can maintain order both on read/write

Done:

  • Replace all instances of native map usage
  • Improve usage of orderedmap to avoid panics
  • Validate some of the changes to tests
  • Test new items can be added to docs in expected order
  • Test existing items in docs can be reordered
  • Improve API for dealing with untyped example/examples fields in schemas

Baliedge and others added 27 commits September 22, 2023 17:49
…ize parallel processing of slices and channels in stable order.
Fix goroutine resource leak in `datamodel/low/v3/path_item.go`.
…ator.

Integrate `TranslateMapParallel()` into datamodel for `Paths` to replace specialized async logic.
Integrate `TranslatePipeline()` into datamodel for schema components to replace specialized async logic.
Revert change to `Scopes` back to `map` type.
`orderedmap.Map` is not properly marshaling this one field from YAML.
@daveshanley
Copy link
Member

Let me know when you're ready for me to review.

@TristanSpeakEasy
Copy link
Contributor Author

TristanSpeakEasy commented Dec 12, 2023 via email

@daveshanley
Copy link
Member

I am working my way through it, should be done tomorrow.

Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

Wow, what a PR! Such a huge lift to turn this around, but provides a real maturity to the library, allowing us to march forward with other higher level features, knowing the lower levels are now solid.

datamodel/high/node_builder.go Outdated Show resolved Hide resolved
datamodel/high/node_builder.go Outdated Show resolved Hide resolved
datamodel/high/node_builder_test.go Show resolved Hide resolved
datamodel/high/v2/path_item.go Show resolved Hide resolved
datamodel/high/v3/callback.go Show resolved Hide resolved
orderedmap/builder.go Show resolved Hide resolved
orderedmap/builder.go Show resolved Hide resolved
orderedmap/orderedmap.go Show resolved Hide resolved
orderedmap/orderedmap.go Show resolved Hide resolved
utils/utils.go Show resolved Hide resolved
@daveshanley
Copy link
Member

In regards to coverage, I would really appreciate bringing it up to 99.6 at a minimum. I don't want to lose the 100% badge we have on the readme, and it rounds up to keep things clean :)

@TristanSpeakEasy
Copy link
Contributor Author

In regards to coverage, I would really appreciate bringing it up to 99.6 at a minimum. I don't want to lose the 100% badge we have on the readme, and it rounds up to keep things clean :)

I'll try and achieve this, some of the uncovered lines were a bit hard to determine the best way to test but I'll try a few techniques and then reach out for help if needed

@TristanSpeakEasy
Copy link
Contributor Author

In regards to coverage, I would really appreciate bringing it up to 99.6 at a minimum. I don't want to lose the 100% badge we have on the readme, and it rounds up to keep things clean :)

Okay coverage seems to be at 99.60% now so hopefully good to go? @daveshanley

@TristanSpeakEasy
Copy link
Contributor Author

I have pull requests ready to go in libopenapi-validator and vacuum once this merges and I can update those dependencies

Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

Looks good, I think you need to rebase however, there are conflicts.

@daveshanley daveshanley merged commit a912093 into pb33f:main Dec 15, 2023
1 check passed
@TristanSpeakEasy TristanSpeakEasy deleted the ordered-libopenapi branch December 15, 2023 15:46
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.

3 participants