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

FE2 API Updates #348

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

JR-Morgan
Copy link
Member

@JR-Morgan JR-Morgan commented Oct 18, 2024

Added New Resources:

  • project_resource.py
  • project_invites_resource.py
  • model_resource.py
  • version_Resource.py
  • comment_resource.py - WON'T DO - I need blob support in order to integration test. Not super critical
  • subscription_resource.py

All of the above are implemented the same as in C# (I've tweaked the C# side also in specklesystems/speckle-sharp-sdk#144 )

Notable improvements over previous aproach:

  • Object models have their nullability (Optional) aligned with the GraphQL schema
  • Simplified the response parsing, and everything is properly typed
  • Exceptions thrown rather than returned

Changes to existing resources:

  • active_user::Resource & other_user::Resource renamed to active_user_resource::ActiveUserResource and other_user_resource::OtherUserResource
  • Deprecated all FE1 queries and mutations

I should be non-breaking, except in a few places I've changed the type hinting to correctly respect the Optionality of things,
and the active_user and other_user resources will now raise rather than return exceptions

Tests

Added pytest equivalents for the C# integration tests for the same queries and mutations
Had to add pytest-asyncio as a dev dependency

Things to check with Gergo:

  • Q: Cursors... str, datetime or Union[str, datetime] - A: str
  • Q: Happy with the integration tests? they are the same as in C# A: more than happy
  • Q: Should I remove the unused stuff (file import, model tree, etc...) Done anyway
  • Q: Project invites in with Project and Active user? or in separate place like in .NET SDK? align with sharp
  • Q: Keep models in new_models.py or move them all to models.py, I've had to suppress ruff is complaining about * imports
  • Q: User Update overloads
  • Q: Happy with the slight renaming of the resources classes
  • Q: old api.models <- this was only around for backwards compatibility right? I don't need to touch it

Copy link

linear bot commented Oct 18, 2024

@JR-Morgan JR-Morgan changed the title Added project resource and fe2 models FE2 API Updates Oct 25, 2024
@JR-Morgan JR-Morgan marked this pull request as ready for review October 30, 2024 15:43
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 98.83721% with 15 lines in your changes missing coverage. Please review.

Project coverage is 89.63%. Comparing base (f1b5184) to head (170d2f0).

Files with missing lines Patch % Lines
...core/api/resources/current/active_user_resource.py 83.72% 7 Missing ⚠️
src/specklepy/core/api/models/current.py 96.61% 4 Missing ⚠️
...lepy/api/resources/current/active_user_resource.py 93.33% 2 Missing ⚠️
src/specklepy/core/api/client.py 93.33% 1 Missing ⚠️
...ore/api/resources/current/subscription_resource.py 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
+ Coverage   87.65%   89.63%   +1.97%     
==========================================
  Files          95      128      +33     
  Lines        5670     6809    +1139     
==========================================
+ Hits         4970     6103    +1133     
- Misses        700      706       +6     

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

@JR-Morgan
Copy link
Member Author

JR-Morgan commented Oct 30, 2024

image
Test coverage

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.

1 participant