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

Quickstart Things #1064

Merged
merged 41 commits into from
Sep 5, 2023
Merged

Conversation

egekorkan
Copy link
Member

@egekorkan egekorkan commented Aug 27, 2023

fixes #1063

and also addresses w3c/wot-cg#67

It is not done yet since:

  • I have TS errors on the coffee machine
  • I need to implement intervals on the smart clock
  • I need to test the behavior
  • I need to understand how to fix the resolve type error (see comment below)

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.05% ⚠️

Comparison is base (3a74512) 75.38% compared to head (efcf67b) 75.33%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1064      +/-   ##
==========================================
- Coverage   75.38%   75.33%   -0.05%     
==========================================
  Files          80       80              
  Lines       15443    15452       +9     
  Branches     1477     1477              
==========================================
  Hits        11641    11641              
- Misses       3766     3775       +9     
  Partials       36       36              

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@egekorkan egekorkan marked this pull request as ready for review August 30, 2023 19:09
@egekorkan
Copy link
Member Author

@relu91 and @danielpeintner I am aware of the failing eslint but I think I am not understanding how to work with the InteractionOutput type. In the simple coffee machine, I want to return a Promise since the action takes a while. I did not manage to avoid the error by trying different combinations. I have put the ts-ignore statements in order to test the behavior.

Other than this:

  • The behavior of the devices are as intended. They are super simple to test, which was one of the goals of this implementation.
  • I did not copy them over to root level examples folder yet. Before I do that, I will change the sensor to MQTT and the clock to CoAP.

@danielpeintner
Copy link
Member

@relu91 and @danielpeintner I am aware of the failing eslint but I think I am not understanding how to work with the InteractionOutput type. In the simple coffee machine, I want to return a Promise since the action takes a while. I did not manage to avoid the error by trying different combinations. I have put the ts-ignore statements in order to test the behavior.

I removed ts-ignore statements and pushed an update (see 511602d). Please let me know whether it works as expected.

BTW, there are several log statements that need to be removed.. I think

  • console.log("here");
  • console.log("here2");
  • console.log("here3");
  • console.log("here4");

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

Please remove unnecessary log statements

packages/examples/src/quickstart/simple-coffee-machine.ts Outdated Show resolved Hide resolved
packages/examples/src/quickstart/simple-coffee-machine.ts Outdated Show resolved Hide resolved
packages/examples/src/quickstart/simple-coffee-machine.ts Outdated Show resolved Hide resolved
packages/examples/src/quickstart/simple-coffee-machine.ts Outdated Show resolved Hide resolved
packages/examples/src/quickstart/simple-coffee-machine.ts Outdated Show resolved Hide resolved
@egekorkan
Copy link
Member Author

@danielpeintner I have removed the log statements, sorry for oversight. However, your refactoring with the return statements broke the time behavior. Now, the request is immediately answered, before the action is finished.

@danielpeintner
Copy link
Member

danielpeintner commented Aug 31, 2023

@danielpeintner I have removed the log statements, sorry for oversight. However, your refactoring with the return statements broke the time behavior. Now, the request is immediately answered, before the action is finished.

I think the latest commits should fix the issue...

@egekorkan egekorkan requested a review from sebastiankb as a code owner August 31, 2023 21:11
@egekorkan
Copy link
Member Author

@danielpeintner your contributions fixed the issues. I did:

  • Changed clock to CoAP
  • I have noticed an old example in mqtt binding readme and fixed it
  • There is are weird problems with MQTT:
    • The import has an error but I have added an ignore statement.
    • There is no connection to the broker. The TD is not logged and events are not emitted. The example in the README works just fine. I feel like I am not seeing something...

@danielpeintner
Copy link
Member

danielpeintner commented Sep 1, 2023

There is are weird problems with MQTT:

  • The import has an error but I have added an ignore statement.

We should keep track of these ignore statements. We used to have #534 for it.
Anyhow, we can open another issue otherwise it gets forgotten and I think we should look into "what" causes the issue

@egekorkan
Copy link
Member Author

@danielpeintner the MQTT example was working the whole time, I had changed the file name and was running the old js file the whole time... 🤦

@egekorkan
Copy link
Member Author

Given that the tests are also passing, I would like to merge this asap and deploy it to have it ready for TPAC. @danielpeintner @relu91 any further changes/review?

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

There are some things we can/should fix/integrate. Otherwise the PR looks good to me. Let's wait for @relu91

packages/examples/README.md Outdated Show resolved Hide resolved
packages/examples/src/quickstart/presence-sensor.ts Outdated Show resolved Hide resolved
packages/examples/src/quickstart/simple-coffee-machine.ts Outdated Show resolved Hide resolved
packages/examples/src/quickstart/smart-clock.ts Outdated Show resolved Hide resolved
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

As said in the comment above before merging we should copy the js files to the examples folder. I reviewed the code I think it is a good toy example, but in the next few days I'll try to use the Things lives and I might update it with fixes and improvements.

@egekorkan
Copy link
Member Author

@danielpeintner @relu91 I have implemented the feedback and copied over to the root examples folder.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

You forgot to remove the first line, I added suggestions so that I can do a quick commit.

examples/quickstart/presence-sensor.js Outdated Show resolved Hide resolved
examples/quickstart/simple-coffee-machine.js Outdated Show resolved Hide resolved
examples/quickstart/smart-clock.js Outdated Show resolved Hide resolved
@relu91
Copy link
Member

relu91 commented Sep 5, 2023

I think I'm going to squash all to commits.

@relu91 relu91 merged commit 67f3a44 into eclipse-thingweb:master Sep 5, 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.

Quick Start Tutorial Things
3 participants