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: add more v1 methods to v2 client #55

Merged
merged 2 commits into from
Feb 24, 2025

Conversation

sirewix
Copy link
Contributor

@sirewix sirewix commented Feb 11, 2025

No description provided.

@sirewix sirewix requested a review from a team as a code owner February 11, 2025 21:04
@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch from 3095b5f to 991987d Compare February 11, 2025 21:19
@sirewix sirewix changed the title feat: add list_project_grant_members method feat: add list_project_grant_members method Feb 11, 2025
@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch from 991987d to 3d3fdf8 Compare February 11, 2025 21:24
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.00%. Comparing base (221f1f7) to head (4fd53b5).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff            @@
##            main     #55      +/-   ##
========================================
+ Coverage   3.19%   4.00%   +0.80%     
========================================
  Files        857     856       -1     
  Lines      36616   36535      -81     
========================================
+ Hits        1171    1462     +291     
+ Misses     35445   35073     -372     

see 31 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 221f1f7...4fd53b5. Read the comment docs.

Copy link
Contributor

@tlater-famedly tlater-famedly left a comment

Choose a reason for hiding this comment

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

Nice, but I'm not a big fan of sneaking in a bunch of refactoring with a feature commit.

I avoid being too precise with this because it can be a ton of work for more hairy commits, but in this case it should be really trivial. Can we split this into two commits, one with the refactor and another with the new method?

@sirewix
Copy link
Contributor Author

sirewix commented Feb 12, 2025

I agree, initially I added the support for org id in headers, which required changing all the lines where PaginationHandler was used, so I decided it was a good chance to add one type parameter. And then I rebased my branch and turned out the org id feature was added already

@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch from 3d3fdf8 to d9b5213 Compare February 12, 2025 17:31
@tlater-famedly
Copy link
Contributor

We should try to add proper sorting to these methods upstream.

@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch 2 times, most recently from 5902a82 to 27bb811 Compare February 17, 2025 17:25
Copy link
Contributor

@tlater-famedly tlater-famedly left a comment

Choose a reason for hiding this comment

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

Love the unboxing of the PaginationHandler, thanks for taking your time to do that.

@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch 2 times, most recently from b652bb3 to 3b91640 Compare February 18, 2025 13:35
Copy link
Contributor

@tlater-famedly tlater-famedly left a comment

Choose a reason for hiding this comment

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

Ready to go except for one detail that almost slipped by me?

@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch 2 times, most recently from 9031cd5 to a846e9d Compare February 21, 2025 17:14
@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch from a846e9d to db8877d Compare February 24, 2025 12:02
@sirewix sirewix force-pushed the sirewix/add-list_project_grant_members branch from db8877d to 4fd53b5 Compare February 24, 2025 12:39
@sirewix sirewix changed the title feat: add list_project_grant_members method feat: add more v1 methods to v2 client Feb 24, 2025
@sirewix sirewix merged commit 4fd53b5 into main Feb 24, 2025
5 checks passed
@sirewix sirewix deleted the sirewix/add-list_project_grant_members branch February 24, 2025 13:18
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