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

Added sharepoint site pages from Beta API and made BaseCollectionPaginationResponse auto pageable #506

Closed

Conversation

dabla
Copy link
Contributor

@dabla dabla commented Dec 7, 2023

Added builder methods which allows retrieving sharepoint site pages from the Beta API.
Refactored BaseCollectionPaginationResponse so it's completely generic and is easy to extend wihout the need to re-implement the same method over and over again (DRY). Also made it auto pageable so the user of the client doesn't have to care how the paging works with the @odata.nextLink property. Also added integration and unit tests. For the integration test a tenant_id/client_id/secret should be defined in the CI/CD so we can make the tests run automatically, this CI/CD part has still to be done.

…h an integration test allowing to run multiple async code test cases
…s are fully implemented and we only need to subclass to define the type/entity which is contained within the pageable collection
…the async iterable so that we don't need to care about pagination anymore as this is now handled behind the scenes for you, still a lot of Collection related response classes will have to be migrated using this one.
…in separate modules and defined a common base module which setups the integration test
… which refactor all those classes using the generic BaseCollectionPaginationResponse class without re-implementing the same code over and over again
…lies lazy evaluation which walking through the different modules
@dabla dabla requested a review from a team as a code owner December 7, 2023 18:03
@dabla
Copy link
Contributor Author

dabla commented Dec 7, 2023

@microsoft-github-policy-service agree company="Infrabel"

@isvargasmsft
Copy link
Member

@isvargasmsft
Copy link
Member

Thank you very much @dabla for the contribution. We'll review in a timely manner.

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.

unfortunately we won't be able to accept the contribution as it changes the generated code, which is overwritten weekly.
Plus there is a separate package for the beta API surface

@sebastienlevert
Copy link

sebastienlevert commented Dec 7, 2023

Thanks for this contribution @dabla. As @baywet stated, the beta package for the missing endpoints would be your best starting point. If there are missing endpoints over there also, please let us know.

On this repo, we can't accept PRs on request builders as we are pushing changes weekly using our code generator Kiota. That being said, there are things we might be able to learn and inject in Kiota to ensure we have a great experience with python and Microsoft Graph.

@baywet @shemogumbe @samwelkanda I'd love your perspective on some of the proposals and identify if some of them could be injected in the overall Python generation of Kiota.

@dabla
Copy link
Contributor Author

dabla commented Dec 8, 2023

Thanks for this contribution @dabla. As @baywet stated, the beta package for the missing endpoints would be your best starting point. If there are missing endpoints over there also, please let us know.

On this repo, we can't accept PRs on request builders as we are pushing changes weekly using our code generator Kiota. That being said, there are things we might be able to learn and inject in Kiota to ensure we have a great experience with python and Microsoft Graph.

@baywet @shemogumbe @samwelkanda I'd love your perspective on some of the proposals and identify if some of them could be injected in the overall Python generation of Kiota.

Hello, too bad it can't be accepted which I feared because of the generated code. I'm willing to contribute in the kiota part the modifications I've done here. Indeed the pages are present in the beta package, which I've looked into but then didn't find it because it was located under the items module. Also I think the testing part I've done here could be a good thing for the package, something to consider and which doesn't interfer with the generated code.

@baywet
Copy link
Member

baywet commented Dec 8, 2023

Please create an issue in kiota to discuss the changes you want to make before you spend the time making the changes. I want to make sure we make good use of your time.

As for the tests, unit testing generated code doesn't add much value when the code generator itself is tested, and the hand written code is also unit tested. We also have an integration test pipeline called raptor which effectively tests the SDK against the documentation snippets.

@baywet
Copy link
Member

baywet commented Dec 13, 2023

Closing due to the code changes targeting generated artefacts

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.

5 participants