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

AAS error handling #514

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

SergioCasCeb
Copy link
Contributor

Added a similar error handling and message just like the ones utilized for OpenAPI and AsyncAPI.

@netlify
Copy link

netlify bot commented Oct 13, 2023

Deploy Preview for thingweb-playground ready!

Name Link
🔨 Latest commit 073b8d5
🔍 Latest deploy log https://app.netlify.com/sites/thingweb-playground/deploys/65328ba2ab105d0008ee1414
😎 Deploy Preview https://deploy-preview-514--thingweb-playground.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SergioCasCeb
Copy link
Contributor Author

Implemented minor fixes with the new close tab prompt, so that if a user opens an example while the close prompt is open, it will close the prompt.

@SergioCasCeb
Copy link
Contributor Author

SergioCasCeb commented Oct 13, 2023

Momentarily removed the "Save as" button until HTTPS is implemented for Playground

@@ -198,6 +196,15 @@ function enableAPIConversionWithProtocol(editorInstance) {
showConsoleError("Please insert a TD which uses MQTT!")
}
}

if (AASTab.checked === true) {
if (["mqtt", "mqtts", "http", "https", "coap", "modbus"].some(p => protocolSchemes.includes(p))) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check there, we should not limit it to protocols we are aware of... makes it also difficult to maintain in the future.

Copy link
Member

Choose a reason for hiding this comment

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

The idea is to align them like other tabs (asyncapi, openapi). Ideally, all thr modules should have the same programming api to report back but it's not the case atm

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

So I have done some tests with even BACnet and the transformation still works. We are not able to really test them though (since there is no BACnet AID) so I would be still leaning to not accept unknown protocols. What is your opinion @danielpeintner about that? Will node-wot reject an unknown protocol in the future or will it try best effort?

@danielpeintner
Copy link
Member

What is your opinion @danielpeintner about that? Will node-wot reject an unknown protocol in the future or will it try best effort?

I guess it would be wrong to reject unknown protocols. The current node-wot code does not make any assumption. Hence I agree with your statement "we should accept unknown protocols"

@egekorkan
Copy link
Member

. Hence I agree with your statement "we should accept unknown protocols"

But shouldn't we at least give a warning? It feels a bit wrong to be confident about something that doesn't "exist"

@danielpeintner
Copy link
Member

But shouldn't we at least give a warning? It feels a bit wrong to be confident about something that doesn't "exist"

I don't feel strongly about it and a warning might be okay.
On the other hand, do we warn people in the TD spec about protocols we don't know yet?

generateAAS(editorInstance["_domElement"].dataset.modeId, editorInstance)
AASView.classList.remove("hidden")
} else {
showConsoleError("Please insert a TD which uses HTTP, MQTT, CoAP or Modbus!")
Copy link
Member

Choose a reason for hiding this comment

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

So this should be changed since all protocols technically work. @SergioCasCeb could you remove the error and only display `Protocols other than HTTP, MQTT and Modbus are not officially supported by AID, do not use the resulting AID in production" on the bottom of the JSON button?

@egekorkan
Copy link
Member

On the other hand, do we warn people in the TD spec about protocols we don't know yet?

There are plans to add protocol validation and this will indeed be a question. So far "htv:methodName":"GETTER" is accepted

Removed the protocol error message, and substituted it for a general warning message in the input section on top of the download button
@egekorkan
Copy link
Member

@danielpeintner what do you think now? It is only a gray text on the right hand side

@danielpeintner
Copy link
Member

@danielpeintner what do you think now? It is only a gray text on the right hand side

Fine by me

@egekorkan egekorkan merged commit 72ef82d into eclipse-thingweb:master Oct 23, 2023
11 checks passed
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