chore(binding-http): enable eslint/strict-boolean-expressions

[JKRhb]

Ticks one of the boxes of #1046 :)

[codecov]

Codecov Report

Attention: 91 lines in your changes are missing coverage. Please review.

Comparison is base (7a99bf9) 75.71% compared to head (4624a03) 75.61%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1097      +/-   ##
==========================================
- Coverage   75.71%   75.61%   -0.10%     
==========================================
  Files          80       80              
  Lines       16106    16153      +47     
  Branches     1507     1518      +11     
==========================================
+ Hits        12194    12214      +20     
- Misses       3874     3898      +24     
- Partials       38       41       +3     

Files	Coverage Δ
packages/binding-http/src/http-client-impl.ts	66.01% <60.00%> (-0.25%)	⬇️
packages/binding-http/src/routes/properties.ts	61.53% <0.00%> (ø)
...ackages/binding-http/src/oauth-token-validation.ts	93.02% <50.00%> (-0.55%)	⬇️
packages/binding-http/src/routes/action.ts	58.26% <25.00%> (ø)
packages/binding-http/src/routes/event.ts	17.39% <0.00%> (ø)
...ckages/binding-http/src/routes/property-observe.ts	16.94% <0.00%> (ø)
packages/binding-http/src/routes/common.ts	59.04% <22.22%> (-0.96%)	⬇️
packages/binding-http/src/routes/property.ts	65.41% <22.22%> (-1.00%)	⬇️
...kages/binding-http/src/routes/thing-description.ts	48.40% <26.31%> (+1.91%)	⬆️
packages/binding-http/src/credential.ts	80.35% <58.13%> (-0.59%)	⬇️
... and 1 more

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

[JKRhb]

(Rebased the PR against the latest state of master.)

[relu91]

I selected request changes but they are mostly suggestions and comments. I'm open to changing my mind on the suggested changes, let me know!

[relu91]

Given the comment, this should probably be: if(thing.security == null). Right?

[JKRhb]

As thing.security is a mandatory field, I think this part of the code covers the thing.security.length === 0 case (i.e., the test for thing.security.length > 0 above does not return true and we did not return from the method early).

This could definitely be a bit clearer, though – maybe we can perform some additional cleanups after this PR has been merged?

[relu91]

It is a mandatory field in the TD but not in the ThingExposeInit. In my understanding, there is nothing so far to check if the security is defined. Probably, due to #911. Currently, we just ask the bindings to handle the ExposeThingInit see https://github.com/eclipse-thingweb/node-wot/blob/master/packages/core/src/servient.ts#L38 . So thing.security might be also null at that point in time. Do the new changes take care of this too? 🤔 .

[JKRhb]

Ohh, I see, then this case might not be covered yet! I will double-check this :)

[JKRhb]

Hmm, the parameter passed into the method is of type ExposedThing with security being of type security: string | [string, ...string[]] – so it is mandatory, but I suppose at least one thing that is missing is the case when security is a single string?

[JKRhb]

I tried to provide a fix now in 7a0bd02 – I think that should work for now, however, it seems to me as if the handling of security and securityDefinitions needs to be revisited in general.

[JKRhb]

Aparently, the code has been correct before and did not work anymore after the change, so I reverted it. Seems to me as if the typing of the security field in the ExposedThing class/interface might need to be fixed.

[relu91]

I know that it is counter-intuitive and I agree that the bug is somewhere else (e.g. #911). If you go upwards in the call hierarchy you'll notice that after producing method nobody checks if security is defined, tomorrow I'll try with a practical example. However for the time being we can merge the PR and track this issue in #911 .

[JKRhb]

Hmm, the persisting linting errors are still puzzling me a bit, as I don't get the same errors on my own machine. I think I now remember that they were the reason why I used the === true approach in the first place. It seems to me, though, that some of the errors are caused by the TS config and using ES6 as the compile target.

I think the easiest resolution would be to stick to === true for now and maybe revisit this issue later.

[JKRhb]

Since the updated eslint packages did not solve the issue and to make the history a bit cleaner, I dropped the commits merged in from #1103 and rebased against master.

[JKRhb]

Rebased against master to resolve a conflict after the merging of #1103 – with the latest changes, I think/hope this PR should also be ready for merging now :)

[relu91]

All the comments above have been resolved. Merging. Thanks @JKRhb !!!

[JKRhb]

Awesome, thank you @relu91! :)

Maybe one thing, though: It's probably best to use the squashing strategy when merging PRs like this one, as the fixup! commits I am using are actually intended to be squashed eventually (see, e.g., here). After the discussions we had recently, I refrained from doing so which now, unfortunately, led to some of those commits "leaking" into the main branch. Maybe we could discuss our workflow with handling PRs more generally at one of the upcoming committer meetings? Potential solutions could be to let the person opening the PR merge the PR after receiving the required approvals, provided that it is a committer, or let them also give a final green light before the merge.

Personally, I would prefer if we would allow to also perform some clean-up in the PR (i.e., rebasing, squashing, and reordering of commits) itself after the reviewers gave their approval since having a clean commit history in the main branch is always a nice thing to have, I think. But maybe we could discuss this also at some point (in not too much detail) in a synchronous manner (which would also probably lead to the resolution of #558).

[danielpeintner]

As said before.. rebasing, squashing, and reordering of commits while working on a PR makes it very difficult to follow what has changed between commits.

I guess the only thing we need to remember (when "actually" merging the PR) is set/choose the right option to "squash all commits".
I think on GitHub we can specify the default strategy ... we might ask Eclipse to do this setting... IF EVERYONE AGREES WITH ... which isn't clear from reading #558

Shall we continue the discussion in #558

[JKRhb]

As said before.. rebasing, squashing, and reordering of commits while working on a PR makes it very difficult to follow what has changed between commits.

You are completely right, that should only be done once all reviews have been addressed. Sorry once more for not following that principle before.

I guess the only thing we need to remember (when "actually" merging the PR) is set/choose the right option to "squash all commits". I think on GitHub we can specify the default strategy ... we might ask Eclipse to do this setting... IF EVERYONE AGREES WITH ... which isn't clear from reading #558

Shall we continue the discussion in #558

👍

[relu91]

@JKRhb you are right I simply overlooked it, let's discuss it on #558.