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

feat: refine promise usages in smart coffee machine scripts #337

Merged

Conversation

fatadel
Copy link
Contributor

@fatadel fatadel commented Sep 24, 2020

Part of #331
Signed-off-by: fatadel [email protected]

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.

I am more in favor of nesting code with if/else so that I can be sure that a resolve or reject is the last possible command executed.

However if you want to keep your code style I suggest adding a return statement after resolve/reject if yet another resolve/reject can follow.

see https://stackoverflow.com/questions/32536049/do-i-need-to-return-after-early-resolve-reject

@fatadel
Copy link
Contributor Author

fatadel commented Sep 25, 2020

I am more in favor of nesting code with if/else so that I can be sure that a resolve or reject is the last possible command executed.

However if you want to keep your code style I suggest adding a return statement after resolve/reject if yet another resolve/reject can follow.

see https://stackoverflow.com/questions/32536049/do-i-need-to-return-after-early-resolve-reject

Done!

@@ -612,6 +612,48 @@ class WoTServerTest {
expect(thing).to.have.property("@context").to.deep.include({"@language": "xx"});
}

@test async "should reject reading property without uriVariables when they are mandatory"() {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you @fatadel . You are right this is really an issue.
I will fix it in another PR to better keep track.

May I ask you to remove the test case in this pr. Sorry indeed and thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no worries!

Signed-off-by: fatadel <[email protected]>
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.

Looks good to me.

Whether you want to use async and await as mentioned by @relu91 here is up to you.

Just let me know whether you want to make these changes or you prefer keeping it as is.

@fatadel
Copy link
Contributor Author

fatadel commented Sep 28, 2020

Looks good to me.

Whether you want to use async and await as mentioned by @relu91 here is up to you.

Just let me know whether you want to make these changes or you prefer keeping it as is.

I would prefer to keep it as it is. async/await might make the code more understandable but for now it is consistent with other promise usages in this example (and also in others I think).

@danielpeintner
Copy link
Member

Makes sense to me since we now have both possibilities in the examples directory (the counter uses async-await on request, see #335)

@danielpeintner danielpeintner merged commit 8eea1ec into eclipse-thingweb:master Sep 29, 2020
@fatadel fatadel deleted the refine-coffee-promises branch October 10, 2020 17:10
@fatadel fatadel restored the refine-coffee-promises branch December 18, 2020 23:46
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.

2 participants