-
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
propagate CompositionOptions to fetchAffordances and _getPartialTDs #57
Conversation
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.
Thank you @dvalnn, changes look good. Just two things left:
- Sign the ECA (see https://github.com/eclipse-thingweb/node-wot/blob/master/CONTRIBUTING.md#legal-requirements)
- Can you please rebase and use a commit message style according to https://github.com/eclipse-thingweb/node-wot/blob/master/CONTRIBUTING.md#commits
@danielpeintner @egekorkan we should a Contributing.md file here too.
…d by fetchAffordances
In the meantime, I squashed and reworded the commits so if everything looks good, I'll just wait for feedback regarding the ECA issue. |
Thank you! to be honest, this is the first time we see that. I have an email containing a "." too. Maybe, it is just some recent regression. We'll try to contact the webmaster. I'll let you know asap. |
@dvalnn I still have not received a response from the eclipse webmaster, but I tried to create a new account with [email protected], and it worked. Can you try again? maybe it was just some temporary issue... |
I finally managed to sign the ECA (and create an account) by just suppressing the ".", from my email (which works the same as having the "." in it seems) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 65.23% 69.33% +4.10%
==========================================
Files 1 1
Lines 558 300 -258
Branches 224 79 -145
==========================================
- Hits 364 208 -156
+ Misses 155 70 -85
+ Partials 39 22 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Ok we are almost there, can you run priettier? Thanks! |
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.
Looks good! Should we bump the version already?
we can do it later |
Change ThingModelHelpers.fetchAffordances() to take CompositionOptions as input and propagate them to successive _getPartialTDs calls.
This allows for correct replacement of placeholders via the CompositionOptions map in Thing Models fetched via fetchAffordances.
Example use case:
Base thing model with placeholders
Another thing model that extends the base thing model
map:
In this scenario, given a valid map, the current implementation will fail to correctly instantiate the partial Thing Description for the "extended_thing" model, as the composition options are not propagated by fetchAffordances() to the following _getPartialTDs() calls.