refactor(coap-server): refactor request handling

[JKRhb]

This PR applies some additional refactoring to the CoapServer class, removing some nesting from the handleRequest method. In follow-up PRs, the logic of this and other methods needs to be further refactored in order to reduce complexity and make the class more maintainable.

[codecov-commenter]

Codecov Report

Merging #994 (de07c04) into master (a005e1f) will increase coverage by 0.15%.
Report is 8 commits behind head on master.
The diff coverage is 72.61%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
+ Coverage   75.61%   75.77%   +0.15%     
==========================================
  Files          72       80       +8     
  Lines       14887    15290     +403     
  Branches     1431     1444      +13     
==========================================
+ Hits        11257    11586     +329     
- Misses       3597     3671      +74     
  Partials       33       33              

Files Changed	Coverage Δ
packages/binding-coap/src/coap-server.ts	79.58% <72.61%> (+6.56%)	⬆️

... and 10 files with indirect coverage changes

[JKRhb]

I think this PR should now finally be ready for review :) Unfortunately, the PR size turned out to be a lot larger than originally intended. However, I think the code should now be more concise and hopefully also more readable, which should make it easier to extend the CoapServer in the future.

Some helper functions might be able to be generalized and moved to the core package in follow-up PRs. Also, there are still some workarounds in the code referring to bugs or missing features in node-coap, some of which might be fixed in the meantime. I will investigate that and will otherwise – if the issues should still be present – try to resolve them.

[relu91]

I have only questions but the content of the PR looks good!

[danielpeintner]

PR LGTM

Some comments below.. really just comments

[relu91]

Reviewed once again, looks good!

[JKRhb]

Reviewed once again, looks good!

Awesome, thank you :) I just squashed the commits into one – the PR would be ready to merge when you are :) CC @danielpeintner

[relu91]

So in general, we had this non-written rule that once two committers agree on a PR we can merge it, but given the fact that daniel explicitly comment on this PR I would wait. It is not fixing any critical bug and there is no urgency. Is that ok @JKRhb?

[JKRhb]

So in general, we had this non-written rule that once two committers agree on a PR we can merge it, but given the fact that daniel explicitly comment on this PR I would wait. It is not fixing any critical bug and there is no urgency. Is that ok @JKRhb?

Sure, from my side we can wait for Daniel's final approval :)

[danielpeintner]

PR looks good, thanks! I will merge it right away ...

Awesome, thank you :) I just squashed the commits into one – the PR would be ready to merge when you are :) CC @danielpeintner

Side comment: I think we should avoid squashing commits unless there is a good reason.
Why do I say that? Squashing commits does no longer allow reviewers to check what has been updated recently. Hence, one needs to re-check everything again...
Having said that, I like it to squash the commits when merging the PR because doing so

1. Reviewer still see the history
2. The commit in master/main is still clean and just contains one commit even if the PR evolved a lot.

What do people think. Shall we setup some kind of rules for that?

[relu91]

Having said that, I like it to squash the commits when merging the PR because doing so

Do you mean I don't? sorry just to understand better your point.

[danielpeintner]

What I would like to propose is the following.

• while we work on a PR and people are reviewing we should not squash commits
• once we agree to merge the PR we can (and maybe should) squash all the commits into one. Hence we squash in the very last step.

[relu91]

I'm ok with that, but only if the changes are closely related. I would not squash if the PR is fixing two issues or dealing with multiple aspects of the code base (e.g. refactoring + fixing).

[JKRhb]

You are right @danielpeintner, sorry for squashing a bit prematurely :/ Next time, I will wait until all final approvals are given.

I guess as a general rule, maybe we can say that squashing should only be done when the reviewers gave their approval/ask a PR author to do so or during the merge process (as you've shown above).