Skip to content

Conversation

metehanunal
Copy link

Model conversion and test are done

Copy link
Contributor

@jakebeal jakebeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contents are good; style and testing needs improvement.

@@ -1,3 +1,4 @@
import sbol2.model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import should not be needed, given the from sbol2 line below.
In fact, we should probably remove that one too since it makes the references in the code more ambiguous.

self.doc2.add(model2)
# Map over all other TopLevel properties and extensions not covered by the constructor
self._convert_toplevel(model3, model2)
# seq2 = sbol2.Sequence(seq3.identity, seq3.elements,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this commented out code.

encoding2 = encoding_map.get(seq3.encoding, seq3.encoding)
# Make the Sequence object and add it to the document
seq2 = sbol2.Sequence(seq3.identity, seq3.elements, encoding=encoding2, version=self._sbol2_version(seq3))
seq2 = sbol2.Sequence(seq3.identity, seq3.elements,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't change the line-wrapping standard on the project. Per the setup.py file at the top, this project uses max-line-length = 119.
There should not be any change to this line of code, since it's just a line-wrapping change.

def visit_model(self, model2: sbol2.model.Model):
# Priority: 3
raise NotImplementedError('Conversion of Model from SBOL2 to SBOL3 not yet implemented')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace violates python conventions.

Suggested change


# Map over all other TopLevel properties and extensions not covered by the constructor
self._convert_toplevel(model2, model3)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much whitespace here; please use pycodestyle to follow standard python conventions..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't currently used. Please either incorporate into a test or delete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't currently used. Please either incorporate into a test or delete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't currently used. Please either incorporate into a test or delete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't currently used. Please either incorporate into a test or delete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to sorted N-triples format, in order to allow direct file-to-file comparison around the loop.
Sorted N-triples guarantees consistent serialization, but TTL does not.

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.

3 participants