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

refactor: align with eslint rules #2 #520

Merged
merged 5 commits into from
Sep 28, 2021

Conversation

fatadel
Copy link
Contributor

@fatadel fatadel commented Sep 20, 2021

The second portion of eslint alignments (previous one - #511).

Signed-off-by: fatadel [email protected]

@danielpeintner
Copy link
Member

Thanks @fatadel !
Can you also run npm run format to fix the coding style issues...

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.

Small changes, but it looks good!

@@ -344,6 +346,9 @@ export default class CoapServer implements ProtocolServer {
res.write(content.body);

res.on("finish", function (err: Error) {
if (err) {
console.error(err);
Copy link
Member

Choose a reason for hiding this comment

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

For logging we need to use our convention, I am wondering if we can enforce it somehow with eslint 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "our convention"?
I hope you've understood that I've added that code because of this rule - https://eslint.org/docs/rules/handle-callback-err.

Copy link
Member

Choose a reason for hiding this comment

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

If you take a look at our packages and the console logs you will notice that we add a prefix to the error so that we can easily detect from which binding the error came from...

e.g.,

https://github.com/danielpeintner/thingweb.node-wot/blob/59fe3264bb4cfa19a3c8cddb0f1c68b4685a4a47/packages/binding-coap/src/coap-server.ts#L72-L74

Copy link
Member

Choose a reason for hiding this comment

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

yes exactly, can you add that prefix, please? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah okay, sure! I thought you mean there is some kind of convention for the way we handle callback errors.

Will be fixed now 😊

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. It is used elsewhere in the file also. Or do I miss anything?

Copy link
Member

@relu91 relu91 Sep 27, 2021

Choose a reason for hiding this comment

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

I've fixed it. But a short question - won't there be a problem with referencing this?

Actually, he might be right, cause the whole code is wrapped inside function in this case if I recall correctly this points to the function object. @fatadel can you do a couple of tests with a small script reproducing the situation? Then we can merge this! 😃

Copy link
Member

Choose a reason for hiding this comment

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

while I was here I did the test myself 😄 :

class Test {
    constructor() {
        this.title = "Test";
    }
    testMeFunction(){
        setInterval( function () {
            console.log("function", this.title);
        }, 1000);
    }

    testMeArrow() {
        setInterval( () => {
            console.log("arrow",this.title);
        }, 1000);
    }
}

new Test().testMeFunction();
new Test().testMeArrow();

output:

function undefined
arrow Test
function undefined
arrow Test
function undefined

arrow Test

Consequently, I'd ask @fatadel to change function into an arrow function :)

Copy link
Member

Choose a reason for hiding this comment

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

Note: I guess we need to check other places than also...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@relu91, done!
Should I also check for this inside functions in other places or let's leave it for a separate PR?

packages/binding-coap/test/coap-client-test.ts Outdated Show resolved Hide resolved
@fatadel
Copy link
Contributor Author

fatadel commented Sep 21, 2021

npm run format

Didn't know about this, thank you!

@fatadel
Copy link
Contributor Author

fatadel commented Sep 21, 2021

Thanks @fatadel !
Can you also run npm run format to fix the coding style issues...

Done ✅

@danielpeintner
Copy link
Member

@relu91 do you see any more issues to be 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.

Looks good let's move on.

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