-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/ted 206 #58
Feature/ted 206 #58
Conversation
Codecov Report
@@ Coverage Diff @@
## main #58 +/- ##
=======================================
Coverage 98.83% 98.84%
=======================================
Files 49 50 +1
Lines 1808 1818 +10
=======================================
+ Hits 1787 1797 +10
Misses 21 21
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
But would be nice to add also the service functions not only the adapter into the implementation.
import subprocess | ||
|
||
|
||
class YARRRML2RMLConvertorABC(abc.ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convertor -> converter
""" | ||
|
||
|
||
class YARRRML2RMLConvertor(YARRRML2RMLConvertorABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converter
@abc.abstractmethod | ||
def convert(self, yarrrml_input_file_path: pathlib.Path, rml_output_file_path: pathlib.Path): | ||
""" | ||
This method converts a yarrrml file and writes the result to another rml file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upper case
rml_output_file_path= rml_file_path | ||
) | ||
rml_result = rml_file.read() | ||
assert rml_result == rml_file_result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a safe asset. Not safe at all.
better pick certain concepts that shall be there and split them into multiple assertions.
No description provided.