coapjs / node-coap

CoAP - Node.js style
MIT License
533 stars 156 forks source link

Fix coding style with regard to StandardJS #251

Closed JKRhb closed 3 years ago

JKRhb commented 3 years ago

This PR updates the codebase to StandardJS and fixes some minor issues. It is still a bit WIP as there are some minor issues left that need to be addressed.

This PR is supposed to be merged after (or as an alternative) to #244 and closes #245.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 1133321366


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/middlewares.js 32 33 96.97%
lib/observe_read_stream.js 27 28 96.43%
lib/observe_write_stream.js 38 39 97.44%
lib/outgoing_message.js 46 47 97.87%
lib/parameters.js 15 16 93.75%
index.js 20 22 90.91%
lib/segmentation.js 51 54 94.44%
lib/option_converter.js 77 82 93.9%
lib/polyfill.js 2 7 28.57%
lib/agent.js 254 285 89.12%
<!-- Total: 1060 1149 92.25% -->
Totals Coverage Status
Change from base Build 1117056563: -0.08%
Covered Lines: 1157
Relevant Lines: 1248

💛 - Coveralls
JKRhb commented 3 years ago

@Apollon77 I managed to update the whole codebase to StandardJS now. I am not sure, however, if this PR is really reviewable :/

Apollon77 commented 3 years ago

I did a fly over - the hugh test coverage is key here I think and in this case testing still works which is a good sign.

I would go for it

JKRhb commented 3 years ago

I did a fly over - the hugh test coverage is key here I think and in this case testing still works which is a good sign.

I would go for it

Okay, great :) Then I guess we could close #255 and go for this PR instead. Maybe we could add another rule that overrides StandardJS' two spaces of indentation and replaces them with four. Even though I am personally okay with two spaces you are right that four spaces make lines easier to distinguish.

Apollon77 commented 3 years ago

Then lets do it that way and thank you for seeing my arguments :-)

JKRhb commented 3 years ago

Then lets do it that way and thank you for seeing my arguments :-)

Great, I will soon update this PR accordingly :)

JKRhb commented 3 years ago

@Apollon77 I've applied the new identation now and rebased the PR against master :) I guess it should be able to be merged now after the release of 0.25.0.

JKRhb commented 3 years ago

I am going to rebase this branch with the new changes from the master branch in the upcoming days.

Apollon77 commented 3 years ago

Then rebase here and close #255 right?

JKRhb commented 3 years ago

Then rebase here and close #255 right?

Yeah, you are right :) I now managed to resolve all conflicts and all tests are still passing. Maybe you could do a last fly-over before we merge this thing? :)

JKRhb commented 3 years ago

I added a couple of last minute changes in 28781b3dfbf5387a8b9200a6d04ec3092d19c3ee that should make control flow statements a bit more readable.

Apollon77 commented 3 years ago

Will check next days (on vacation right now, so time is rare)

JKRhb commented 3 years ago

Will check next days (on vacation right now, so time is rare)

No problem, take your time :) I just wanted to get this off my list. Hope you are having a good time :)

Apollon77 commented 3 years ago

too big to really fully review ,but scanned over it ... looks fine, will merge now

Apollon77 commented 3 years ago

PS: We had a great time with noch that much time for other stuff beside "family" ;-))

Today packing and tomorrow driving back 10-15h ;-)

JKRhb commented 3 years ago

too big to really fully review ,but scanned over it ... looks fine, will merge now

Great, thank you! :)

We had a great time with noch that much time for other stuff beside "family" ;-))

Sounds awesome! :) Apart from the driving time of course :/ Have a safe journey home!