-
Notifications
You must be signed in to change notification settings - Fork 80
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
refactor: favor WoT.requestThingDescription instead of general fetch #1183
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1183 +/- ##
=======================================
Coverage 76.66% 76.66%
=======================================
Files 80 80
Lines 16813 16813
Branches 1618 1618
=======================================
Hits 12890 12890
Misses 3893 3893
Partials 30 30 ☔ View full report in Codecov by Sentry. |
Thank you, @danielpeintner, that looks pretty good already :) Let us know if we can support you somehow. |
598140c adds also the changes/updates w.r.t. documentation. Something I noticed. We seem to use different import styles. e.g. HTTP node-wot/packages/binding-http/README.md Lines 29 to 30 in 7334567
vs. CoAP node-wot/packages/binding-coap/README.md Lines 29 to 30 in 7334567
I think we should align the style. Any preference? |
I opened #1187 to tackle this in a follow up PR |
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.
LGTM! :) Maybe one last thing, though: Should we add a @deprecated
tag to the fetch
method to guide people to the new requestThingDescription
method?
I tend to think this is a generic helper method that can be used for any other purpose as well.. we are not claiming it is meant to fetch TDs only. Hence I would argue we can/should keep it as is. What do others think? |
My take is that we have already the fetch function for that. Now it is supported in Node.js too, so no need to define yet another generic fetching function. |
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.
Other than the comment above I think we can merge it.
Is the |
yeah good point, but I would argue that the old |
I think we should use
requestThingDescription()
instead of the genericfetch()
everywhere.Please let me know whether this change makes sense.
fixes #1055