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.

Sign up for GitHub

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

Jump to bottom

Implementation for SBOL2<-->SBOL3 converter #222

Merged
merged 15 commits into from
Oct 31, 2023
Merged

Implementation for SBOL2<-->SBOL3 converter #222

merged 15 commits into from
Oct 31, 2023

Conversation

Yehuda-Binik
Copy link
Collaborator

In the new converter, I completed the visit_implementation function for both the SBOL3To2ConversionVisitor and SBOL2To3ConversionVisitor classes. I also added two test cases to test my code using the SBOL3 implementation.nt file from the SBOLTestSuite.

@jakebeal
Copy link
Contributor

@Yehuda-Binik : there are some files that shouldn't have been checked in that were checked in. Can you please remove the .idea and sbol_utilities.egg-info directories? In general, .idea and *.egg-info should be in your .gitignore files.

@Yehuda-Binik
Copy link
Collaborator Author

@jakebeal I removed the .idea and sbol_utilities.egg-info directories. I also updated my .gitignore Is it necessary to delete my .gitignore file from the pull request as well? Let me know if there is anything left I need to change.

@jakebeal
Copy link
Contributor

Thank you! Adding to .gitignore is fine and helpful. I will review the PR in detail now.

jakebeal
jakebeal requested changes Oct 25, 2023
View reviewed changes
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.

There's some minor cleanup needed - the substance of the changes is all good, I just need you to remove the accidental check-ins and unnecessary space changes.

sbol_utilities/sbol3_sbol2_conversion.py Outdated Show resolved Hide resolved
test/sbol3.nt Outdated Show resolved Hide resolved
test/test_files/sbol3_genbank_conversion/ignoring_sbol_properties.nt.test Outdated Show resolved Hide resolved
test/test_sbol2_sbol3_direct.py Outdated Show resolved Hide resolved
test/test_sbol2_sbol3_direct.py Show resolved Hide resolved
@Yehuda-Binik
Update sbol3_sbol2_conversion.py
972268f 
Removed unnecessary comment
@Yehuda-Binik
Delete test/sbol3.nt
4d09298 
Deleted a file that was accidentally checked in
@Yehuda-Binik
Delete test/test_files/sbol3_genbank_conversion/ignoring_sbol_propert…
dcbda5f 
…ies.nt.test

This file was accidentally checked in.
@Yehuda-Binik
Update test_sbol2_sbol3_direct.py
41f65dd 
Removed unnecessary and superfluous formatting changes.
@Yehuda-Binik
Update test_sbol2_sbol3_direct.py
7750c08 
Removed two commented out lines of code that were useful during testing, but no longer needed
@Yehuda-Binik
Copy link
Collaborator Author

I removed the two accidentally checked-in files deleted 3 unnecessary comments (the unneeded raise NotImplemented comment as well as two comments I had written myself) and removed superfluous formatting changes.

jakebeal
jakebeal approved these changes Oct 31, 2023
View reviewed changes
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.

Code looks good, and the two errors are, I believe, only from the secret-requiring tests that are not affected by this PR

@jakebeal jakebeal merged commit 3ef7723 into SynBioDex:develop Oct 31, 2023
1 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebeal jakebeal jakebeal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Yehuda-Binik @jakebeal