add transformer functions and mappers #2
Conversation
isamples_sesar/Mapper.py
Outdated
There was a problem hiding this comment.
The python recomendation for module (source file) names is to be snakecased (lowercase with underscores). You'll definitely see some examples in iSamples code that don't follow this but I did those before I knew better. Since you're starting fresh I'd go ahead and rename the filename (and you'll need to redo any associated imports).
There was a problem hiding this comment.
There was a problem hiding this comment.
This isn't a hard and fast rule, but we try and conform to these conventions when it makes sense.
| @@ -400,7 +398,7 @@ class Sample(SQLModel, table=True): | |||
| launch_type: Optional["Launch_Type"] = Relationship() | |||
| nav_type: Optional["Nav_Type"] = Relationship() | |||
| additional_names: Optional[List["Sample_Additional_Name"]] = Relationship() | |||
| sample_type: Optional["Sample_Type"] = Relationship( | |||
| sample_type: "Sample_Type" = Relationship( | |||
There was a problem hiding this comment.
I'm curious why this changed.
There was a problem hiding this comment.
Every sample in sesar is required to have a sample type and certain assumptions (about a sample type existing) made in the transformer functions were giving errors in mypy.
isamples_sesar/SesarTransformer.py
Outdated
| sample_type_str = sample_type.name | ||
| keyword_arr.append({ | ||
| "keyword": sample_type_str, | ||
| "scheme_name": "SESAR: Sample Type" |
There was a problem hiding this comment.
Generally speaking I think these schemes are supposed to be URIs. I'll double check what OpenContext is doing.
There was a problem hiding this comment.
this is just copying what i saw in the examples
isamples_sesar/SesarTransformer.py
Outdated
|
|
||
| def id_string(self) -> str: | ||
| return "https://data.isamples.org/digitalsample/{0}/{1}".format( | ||
| 'igsn', self.sample.igsn |
There was a problem hiding this comment.
f strings are the new python coolness, I'd recommend rewriting as
return f"https://data.isamples.org/digitalsample/igsn/{self.sample.igsn}". Under the covers it does the same thing as this. but with much friendlier syntax.
There was a problem hiding this comment.
yeah i'd been using f strings in other spots, but this was probably a copy paste of existing code
examples/EOI00002H.json
Outdated
There was a problem hiding this comment.
It would be good to formalize these as tests. Will post a link to how we do this in iSamples…
There was a problem hiding this comment.
There was a problem hiding this comment.
Let me know if you have any questions on these.
There was a problem hiding this comment.
You would need to make some test files with the raw SESAR source records as well.
isamples_sesar/SesarTransformer.py
Outdated
There was a problem hiding this comment.
Same comment on filename (module name), should be snake case.
|
It would be good to add some unit tests to the database layer using sqlite, here's an example of how we do it in iSamples code: https://github.com/isamplesorg/isamples_inabox/blob/develop/tests/test_sqlmodel_database.py |
SeanSCao
force-pushed
the
transformer
branch
from
d3bf820
to
7b74232
Compare
|
Tests require database connection, should these just be ignored by the github action? |
|
@SeanSCao -- the tests should have a database connection to the in-memory database and run as part of the github action. Are some tests trying to connect to postgresql? That definitely shouldn't be happening. |
|
I think I see what you mean -- you have two test files, one against (presumably) a live postgres db and one against sqlite? If that's what you're asking, then yeah you can ignore the one running against postgres like this: When github runs the action it sets the |
No description provided.