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

add servable editing to sdk #161

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open

add servable editing to sdk #161

wants to merge 18 commits into from

Conversation

isaac-darling
Copy link
Contributor

@isaac-darling isaac-darling commented Aug 19, 2022

Not ready to be merged yet, remains untested. The DLHub Service has a corresponding update that adds the route this method requires. Considerations for deleting servables are also included, but the delete_servable function is not yet implemented.

@ascourtas
Copy link
Contributor

Auth issues are fixed, but fails on incompatible typing syntax in Python 3.7 and 3.8, and in 3.9 and 3.10 we get the following error:

dlhub_sdk/utils/search.py:22: in search
    return super().exclude_field("dlhub.deleted", True).search(q, advanced, limit, info, reset_query)
E   RecursionError: maximum recursion depth exceeded while calling a Python object

Requires additional refactoring before merge is possible

@isaac-darling
Copy link
Contributor Author

I thought this might happen, so I prepared a possible solution. Hopefully it helps :)

@coveralls
Copy link

coveralls commented Sep 16, 2022

Pull Request Test Coverage Report for Build 3064702654

  • 12 of 36 (33.33%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.9%) to 87.972%

Changes Missing Coverage Covered Lines Changed/Added Lines %
dlhub_sdk/tests/test_dlhub_client.py 3 6 50.0%
dlhub_sdk/client.py 8 29 27.59%
Totals Coverage Status
Change from base Build 2929398202: -0.9%
Covered Lines: 1653
Relevant Lines: 1879

💛 - Coveralls

@ascourtas
Copy link
Contributor

putting a pin in this until we have clarity on whether or not we're moving to a serverless architecture

@ascourtas
Copy link
Contributor

addresses issues #134 and #135

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.

4 participants