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

findProtocol SHOULD NOT require base #1270

Open
danielpeintner opened this issue Apr 22, 2024 · 4 comments
Open

findProtocol SHOULD NOT require base #1270

danielpeintner opened this issue Apr 22, 2024 · 4 comments
Labels
Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting.

Comments

@danielpeintner
Copy link
Member

danielpeintner commented Apr 22, 2024

This is done in node-wot at https://github.com/eclipse-thingweb/node-wot/blob/master/packages/td-tools/src/td-helpers.ts#L27 a bit but in general, this kind of code should be in node-wot or td-tools

Originally posted by @egekorkan in eclipse-thingweb/node-red#16 (comment)

@danielpeintner
Copy link
Member Author

danielpeintner commented Apr 22, 2024

At the moment for finding the protocol we check base only. If it is not there it fails.
Certainly we can look into other interactions as well but I wonder "what" should happen if there are multiple protocols in place?

EDIT1: the same goes for findPort(...)
EDIT2: I checked the codebase and neither findProtocol(...) nor findPort(...) is used anywhere in our code!

@egekorkan
Copy link
Member

If not used, we can have it in td-tools repo since playground has its own thing as well at https://github.com/eclipse-thingweb/playground/blob/master/packages/core/index.js#L1293 . It's used to check whether open or async api generation can be done

The code in playground looks quite good actually but I had forgotten about it ^^

@danielpeintner
Copy link
Member Author

My opinion is as follows:

  • I would like to remove those 2 helper functions (since they are buggy)
  • I would not introduce similar functionality anywhere else since the expected results might be different
    • in many cases there is not one protocol and/or port
    • playground reports all schemes which might be fine but I see use-cases that are interested in properties and not actions etc Hence you might end up having a own functionality/code anyway.

Question: are we all okay if I provide a PR removing findPort(...) and findPort(...) from node-wot. We can discuss whether some td-tools projects wants to have it... at least with the signature of returning multiple results and not as we have now.

@egekorkan egekorkan added the Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting. label Apr 26, 2024
@egekorkan
Copy link
Member

We will create a package in td-tools that has such helper functions and then deprecate the functions here. Also, the function in https://github.com/eclipse-thingweb/playground/blob/master/packages/core/index.js#L1293 will be moved there and published. It can be more flexible with some filters like "find it only in properties".

td-utils will be the new package name and can contain such tools. It should return an object with binding name as keys (just http, opc etc) then inside it found uri scheme (http, https, mqtt+ws) and subprotocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting.
Projects
Status: In Progress
Development

No branches or pull requests

2 participants