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

Adding MultiFileTransformer #6

Merged
merged 17 commits into from
May 3, 2023
Merged

Adding MultiFileTransformer #6

merged 17 commits into from
May 3, 2023

Conversation

cmungall
Copy link
Member

@cmungall cmungall commented Apr 23, 2023

The MFT is analogous to linkml-run-examples, but instead of performing conversion, it will perform a transformations, and check the output against what is expected

@ialarmedalien ialarmedalien mentioned this pull request May 2, 2023
@ialarmedalien
Copy link
Collaborator

Did you see that nearly all tests are failing?

I'm going to extract out some of the linting/formatting stuff from this PR to reduce its size a bit.

@cmungall
Copy link
Member Author

cmungall commented May 2, 2023

OK I believe using the latest reference validator will fix, we shall see...

@ialarmedalien
Copy link
Collaborator

are tests passing locally?

@cmungall
Copy link
Member Author

cmungall commented May 2, 2023

OK, it definitely should work now (the tests were previously working locally for me as I had a local copy of changes to linkml-runtime). However, now something is up w.r.t coverage. Not really sure how your PR can be interacting with mine...?

@ialarmedalien
Copy link
Collaborator

ialarmedalien commented May 2, 2023

They aren't interacting. This PR is using the old test/coverage command -- it looks like the coverage package isn't being installed so the coverage command is failing. Tests/coverage should work when the other PR is merged and then this one rebased/merged.

Comment on lines +28 to +29
{%- elif sd.expr -%}
{{ sd.expr }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this generates invalid code ATM -- need to replace {var_name} with var_name

e.g.

def derive_Agent(
        source_object: src.Person
    ) -> tgt.Agent:
    return Person(
       id=source_object.id,
       label=source_object.name,
       age=str({age_in_years})+' years',
       ...
    )

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - the compiler subpackages should come with big health warnings for now, this is more an exploration of what can be done

@@ -40,6 +40,9 @@ classes:
description: human readable title for this transformation specification
prefixes:
description: maps prefixes to URL expansions
range: KeyVal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not specific to this line of the file, but can/should there be a specification of the language used for expr? I know it's supposed to be in the language agnostic linkml expression syntax (cough python cough), but it would be good to explicitly specify this, so that it is then possible to use other languages for expressing these transformations.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should definitely do this! this is now a little easier as we have https://github.com/linkml/semantic-dsl

@@ -106,3 +144,19 @@ def _coerce_to_singlevalued(self, slot_derivation: SlotDerivation, class_derivat
if not slot.multivalued:
return True
return False

@property
def curie_converter(self) -> Converter:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice, thanks!

@cmungall
Copy link
Member Author

cmungall commented May 2, 2023

They aren't interacting. This PR is using the old test/coverage command -- it looks like the coverage package isn't being installed so the coverage command is failing. Tests/coverage should work when the other PR is merged and then this one rebased/merged.

ah ok. should we wait - not sure how much there is to do on the other PR. I would be open to doing a cheeky merge of this PR and then moving to a more robust approach moving forward

@ialarmedalien
Copy link
Collaborator

The other PR is done -- it was mostly to reduce the size of this PR by removing some of the black/isort linting changes.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@eebaa4e). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main       #6   +/-   ##
=======================================
  Coverage        ?   86.51%           
=======================================
  Files           ?       20           
  Lines           ?      749           
  Branches        ?        0           
=======================================
  Hits            ?      648           
  Misses          ?      101           
  Partials        ?        0           

@cmungall cmungall merged commit df40ae6 into main May 3, 2023
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