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

Type annotation for more of the control code #1832

Merged
merged 1 commit into from Feb 1, 2019

Conversation

Projects
None yet
2 participants
@pgunn
Copy link
Contributor

pgunn commented Jan 10, 2019

This exposed some issues; I left some questions in the code that we should converse on (and I'll remove comments then) before it lands.

@@ -242,7 +244,7 @@ def copy_annotations(source_pid, target_pid, import_treenodes=True,
SELECT
FROM connector_treenode ct
WHERE ct.project_id=%s
''' % (target_pid, source_pid))
''' % (target_pid, source_pid)) # FIXME "Not all arguments converted during string formatting"

This comment has been minimized.

@tomka

tomka Jan 18, 2019

Contributor

It looks like the whole SQL call isn't implemented properly. I will need to look at this in more detail. It's likely that this code path hasn't been used in quite a while.

@@ -299,7 +301,7 @@ def copy_annotations(source_pid, target_pid, import_treenodes=True,
JOIN class_instance ci_s ON ci_s.id=cici.class_instance_b
WHERE cici.project_id=%s AND relation_id=%s
''')
except DoesNotExist:
except DoesNotExist: # FIXME: This exception is not defined, although Django has a number of more specific variants

This comment has been minimized.

@tomka

tomka Jan 18, 2019

Contributor

Right, glancing over the code, it should probably be except Class.DoesNotExist, Relation.DoesNotExist: and the code above should only execute the INSERT SQL if annotations_src has elements. But given that the SQL won't work anyway, and hence this code is not used actively, it might not be worth the effort. I will check in more detail if the whole annotationadmin.py functionality can be removed. Exports and imports are currently done through the CLI tools catmaid_export_data and catmaid_import_data.

@tomka

This comment has been minimized.

Copy link
Contributor

tomka commented Jan 18, 2019

Thanks for these changes Pat! I tried to comment on all questions and suggestions. You seem to have found quite a few unused code paths and some broken functionality. Some parts of CATMAID, e.g. the ontology and classification tools, didn't get much testing over the last years.

@pgunn

This comment has been minimized.

Copy link
Contributor Author

pgunn commented Jan 24, 2019

@tomka Hey, so I've addressed all your code review; let me know if it looks good to you now, and if it does I'll squash it down for final review.

--Pat

@tomka

This comment has been minimized.

Copy link
Contributor

tomka commented Jan 29, 2019

Great, I went through your update and all looks good. Will merge the branch once you squashed those changes.

@pgunn pgunn force-pushed the pgunn:dev branch from 3ad3542 to c357567 Jan 30, 2019

@pgunn

This comment has been minimized.

Copy link
Contributor Author

pgunn commented Jan 30, 2019

Squashed

@tomka tomka merged commit 2cb8f29 into catmaid:dev Feb 1, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment