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.

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

File format to extension function #378

Merged
merged 5 commits into from
Feb 10, 2022

Conversation

mohitdmak
Copy link
Contributor

Hey @tcmitchell @jakebeal , this tries to fix #244 , by implementing a function bound on the Document class to provide standard extensions for given file format.

@jakebeal jakebeal requested a review from tcmitchell February 9, 2022 19:31
@tcmitchell
Copy link
Collaborator

This looks great. I made a couple of suggestions to improve the code. I'd also like to see a unit tests that checks at least 2 of the known file types and one unknown file type, verifying the raised exception.

There are unit tests that check for exceptions. Let me know if you can't find one. I would be happy to help you craft some or all of this test method. It should go in test_document.py.

Thanks!

@@ -279,6 +279,22 @@ def _guess_format(self, fpath: str):
rdf_format = 'nt11'
return rdf_format

# Return standard extensions when provided the document's file format
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this comment line and put it into a docstring (see below):

Suggested change
# Return standard extensions when provided the document's file format

@@ -279,6 +279,22 @@ def _guess_format(self, fpath: str):
rdf_format = 'nt11'
return rdf_format

# Return standard extensions when provided the document's file format
def file_extension(self, file_format: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this a staticmethod since it doesn't use self, and add a docstring:

Suggested change
def file_extension(self, file_format: str) -> str:
@staticmethod
def file_extension(file_format: str) -> str:
"""Return standard extensions when provided the document's file format
:param file_format: The format of the file
:return: A file extension, including the leading '.'
"""

mohitdmak and others added 3 commits February 10, 2022 09:15
Co-authored-by: Tom Mitchell <tcmitchell@users.noreply.github.com>
@mohitdmak
Copy link
Contributor Author

This looks great. I made a couple of suggestions to improve the code. I'd also like to see a unit tests that checks at least 2 of the known file types and one unknown file type, verifying the raised exception.
There are unit tests that check for exceptions. Let me know if you can't find one. I would be happy to help you craft some or all of this test method. It should go in test_document.py.

Sure, I have made one named test_file_extension in test_document.py.

@tcmitchell tcmitchell self-requested a review February 10, 2022 16:18
@tcmitchell tcmitchell merged commit 7bcfc39 into SynBioDex:main Feb 10, 2022
@tcmitchell tcmitchell self-assigned this Feb 10, 2022
@tcmitchell tcmitchell added this to the 1.0 milestone Feb 10, 2022
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.

Filetype to extension converter
2 participants