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

chore: update to use iterators on orderedmaps #319

Merged
merged 6 commits into from
Aug 26, 2024

Conversation

TristanSpeakEasy
Copy link
Contributor

@TristanSpeakEasy TristanSpeakEasy commented Aug 14, 2024

This PR adds support for the iterators being added to the ordered map library, this cleans up the DevEx when working with ordered maps in the high level models but has also cleaned up a bunch of the implementation code as well

I have linked to my fork of the iterators change for now, but I can hold of this PR (just let me know) until its merged into the library

wk8/go-ordered-map#41

Just some notes about things I noticed while doing this refactor:

  • There is a lot of duplicated code that could be reduced with some helper methods, I added some but stopped short of going to far in one PR
  • The way maps are hashed is very inconsistent and in a lot of cases changing key names (as long as they don't change the order of the sorted map) wouldn't be caught in a hash change, worth probably doing a pass on this in a separate PR

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.65%. Comparing base (c3eb16d) to head (ae88d35).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #319      +/-   ##
==========================================
+ Coverage   99.63%   99.65%   +0.01%     
==========================================
  Files         164      164              
  Lines       16608    16586      -22     
==========================================
- Hits        16547    16528      -19     
+ Misses         56       53       -3     
  Partials        5        5              
Flag Coverage Δ
unittests 99.65% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TristanSpeakEasy TristanSpeakEasy marked this pull request as draft August 15, 2024 11:30
@TristanSpeakEasy
Copy link
Contributor Author

a draft until the underlying dependency is merged otherwise we either need to use speakeasy's fork without a replace statement or everyone needing to use this version has to add their own replace statement

@daveshanley
Copy link
Member

I will need to look at this in depth, thank you for this work!

@TristanSpeakEasy
Copy link
Contributor Author

I will need to look at this in depth, thank you for this work!

@daveshanley we have integrated this branch into the speakeasy products and its working as expected and much nicer devex

@TristanSpeakEasy TristanSpeakEasy marked this pull request as ready for review August 15, 2024 15:44
@TristanSpeakEasy
Copy link
Contributor Author

no longer using a replace directive so good to go

@petkostas
Copy link

I started testing the specific PR, and it looks like it is working as expected! Thanks a lot, @TristanSpeakEasy

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.

This looks great. A very nice upgrade.

@daveshanley daveshanley merged commit 49a2921 into pb33f:main Aug 26, 2024
4 checks passed
pierre-emmanuelJ pushed a commit to exoscale/egoscale that referenced this pull request Sep 12, 2024
# Description

The update of the Go version to go1.23 forces all the users of the API
client to upgrade to go1.23.

A lib should avoid forcing to upgrade Go for compatibility reasons,
especially in a bug fix.

The update of Go is related to the update of
`github.com/pb33f/libopenapi` to v0.18.0 inside the PR
#656.

`github.com/pb33f/libopenapi@v0.18.0` uses go1.23 because of the usage
of the new `iter` package from go1.23
(pb33f/libopenapi#319).

This is only related to the generator package, this package is not used
by the users of the API client.

So I created a dedicated module for the generator: the dependencies of
the generator will be isolated inside this module, and will have no
impact on the API client.

This module doesn't need release, it's just a "local" module.

The target `make generate` still works as before.

This will not impact users who have already updated to go1.23.

---

Side note:

The usage of the vendoring for a lib is mainly useless because the
vendor folder is not included inside the module distributed by the Go
proxies.
Also, Go proxies are here to ensure that a lib cannot disappear: if the
GitHub repository is deleted, the Go proxies will still serve the module
(even if it's a pseudo-version).
So I recommend removing the vendor directories (at least for v3 and the
generator).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants