[SYNPY-1415] Folder model finishing touches #1061
Conversation
…b85504e77a17c213' already exists
|
Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-02-12 16:56:25 UTC |
BryanFauble
changed the title
| @@ -12,6 +12,7 @@ repos: | |||
| - id: check-merge-conflict | |||
| - id: check-xml | |||
| - id: check-yaml | |||
| exclude: ^mkdocs\.yml$ | |||
There was a problem hiding this comment.
format: !!python/name:pymdownx.superfences.fence_code_format is not valid syntax according this this check-yml check - However, this is the format required for it to work: https://squidfunk.github.io/mkdocs-material/reference/diagrams/#configuration
| @@ -2342,27 +2341,28 @@ def getChildren( | |||
|
|
|||
| - [synapseutils.walk][] | |||
| """ | |||
| parentId = id_of(parent) if parent is not None else None | |||
| with tracer.start_as_current_span("Synapse::getChildren"): | |||
There was a problem hiding this comment.
I thought we were using the decorator?
There was a problem hiding this comment.
The decorator is not working right for generators - This was the most straight forward solution at the moment to get it working.
| @@ -15,6 +15,10 @@ class SynapseFileNotFoundError(SynapseError): | |||
| """Error thrown when a local file is not found in Synapse.""" | |||
|
|
|||
|
|
|||
| class SynapseNotFoundError(SynapseError): | |||
There was a problem hiding this comment.
Nit: does python base exceptions have something for Not found errors?
There was a problem hiding this comment.
I did not find one - The closet from: https://docs.python.org/3/library/exceptions.html
were:
- FileNotFoundError - Derived from OSError and not applicable in all cases
- ModuleNotFoundError - Not really the same thing
| @@ -1,5 +1,9 @@ | |||
| """References to the mixins that are used in the Synapse models.""" | |||
|
|
|||
There was a problem hiding this comment.
Out of scope for this ticket but I think there is also a versionable mixin that currently exists in the client.
There was a problem hiding this comment.
There is this Versionable: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/entity.py#L23
Which is focused on extending data (Multiple Inheritance) rather than extending functionality.
For these models I am already explicitly adding the fields to the dataclasses themselves:
https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/synapseclient/models/file.py#L256
It may make sense later on to add a mixin for doing version specific operations later on though - (Read, Delete) for those that support it (Like Table, File)
| @@ -63,13 +63,13 @@ def test_json_schema_organization(js): | |||
|
|
|||
|
|
|||
| class TestJsonSchemaSchemas: | |||
| @pytest.fixture(autouse=True) | |||
| @pytest.fixture(autouse=True, scope="function") | |||
There was a problem hiding this comment.
This is an effort to resolve these integration test errors:
Semantic version: '0.48786.1' already exists for this JSON schema
That have been popping up. These passed on my local machine - Will be verifying they also pass in the CI pipeline
There was a problem hiding this comment.
Can confirm these tests also passed on my machine.
There was a problem hiding this comment.
This looks good. Integration tests pass and I can confirm the poc script for the Folder model works. I had a few comments, but if anything comes out of them, I think it can be for another PR. Don't see anything getting in the way of having this one merged for validation.
Just a note: The provenance of copied entities gets updated with where they were copied from and the version appears to reset, which I believe is expected behavior(?) but want to post here in case not
|
Problem:
Solution:
Testing:
synapseutils.syncFromSynapseon a project with 10 files/9 directories:async: 1.9427805209998041
syncFromSynapse: 3.578380188002484