Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

nanocoap: rename coap_get_code() -> coap_get_code_decimal() #20004

Merged
merged 3 commits into from
Nov 28, 2023
Merged

nanocoap: rename coap_get_code() -> coap_get_code_decimal() #20004

merged 3 commits into from
Nov 28, 2023

Conversation

benpicco
Copy link
Contributor

Contribution description

This avoids the confusion where coap_get_code() is used by default because of it's convenient name, but returns different values than those used to build the response.

This also converts some unnecessary uses of this function to rather use coap_get_code_raw() - when processing the request method, both functions return the same result as the request class is 0.

Testing procedure

Should not change any behavior.

Issues/PRs references

@benpicco benpicco requested review from chrysn and bergzand October 20, 2023 13:21
@benpicco benpicco added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 20, 2023
@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Oct 20, 2023
@benpicco benpicco force-pushed the coap_get_code_decimal branch from 29b6c35 to 7a425c2 Compare October 20, 2023 13:22
@benpicco benpicco added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Oct 20, 2023
chrysn
chrysn reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Can't we just remove it?

All we had in terms of users in the code base are tests, and if we're happy with the breakage of the API change of the transitional period, let's get rid of it right away. (On the long run we might even reuse the coap_get_code for coap_get_code_raw, but that works best after a release that did not have that symbol, as just altering the behavior would lead to silent breakage with users.)

tests/unittests/tests-gcoap/tests-gcoap.c
@@ -89,7 +89,7 @@ static void test_gcoap__client_get_req(void)
len = gcoap_request(&pdu, &buf[0], CONFIG_GCOAP_PDU_BUF_SIZE,
COAP_METHOD_GET, &path[0]);

TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pdu));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_raw(&pdu));

tests/unittests/tests-gcoap/tests-gcoap.c
@@ -157,7 +157,7 @@ static void test_gcoap__client_put_req(void)

coap_parse(&pdu, buf, len + 1);

TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code_decimal(&pdu));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code_decimal(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code_raw(&pdu));

tests/unittests/tests-gcoap/tests-gcoap.c
@@ -223,7 +223,7 @@ static void test_gcoap__client_ping(void)
NULL);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(COAP_CODE_EMPTY, coap_get_code(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_CODE_EMPTY, coap_get_code_decimal(&pdu));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_CODE_EMPTY, coap_get_code_decimal(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_CODE_EMPTY, coap_get_code_raw(&pdu));

tests/unittests/tests-gcoap/tests-gcoap.c
@@ -261,7 +261,7 @@ static void test_gcoap__server_get_req(void)
int res = _read_cli_stats_req(&pdu, &buf[0]);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pdu));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_raw(&pdu));

tests/unittests/tests-gcoap/tests-gcoap.c
@@ -333,7 +333,7 @@ static void test_gcoap__server_con_req(void)
int res = _read_cli_stats_req_con(&pdu, &buf[0]);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pdu));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pdu));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_raw(&pdu));

tests/unittests/tests-nanocoap/tests-nanocoap.c
@@ -146,7 +146,7 @@ static void test_nanocoap__put_req(void)

coap_pkt_init(&pkt, &buf[0], sizeof(buf), len);

TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code_decimal(&pkt));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code_decimal(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_PUT, coap_get_code_raw(&pkt));

tests/unittests/tests-nanocoap/tests-nanocoap.c
@@ -683,7 +683,7 @@ static void test_nanocoap__server_get_req(void)
int res = _read_riot_value_req(&pkt, &buf[0]);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pkt));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_raw(&pkt));

tests/unittests/tests-nanocoap/tests-nanocoap.c
@@ -740,7 +740,7 @@ static void test_nanocoap__server_get_req_con(void)
int res = _read_riot_value_req_con(&pkt, &buf[0]);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pkt));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_decimal(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_METHOD_GET, coap_get_code_raw(&pkt));

tests/unittests/tests-nanocoap/tests-nanocoap.c
@@ -1081,7 +1081,7 @@ static void test_nanocoap__token_length_ext_16(void)
int res = coap_parse(&pkt, buf, 21);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(204, coap_get_code(&pkt));
TEST_ASSERT_EQUAL_INT(204, coap_get_code_decimal(&pkt));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(204, coap_get_code_decimal(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_CODE_CHANGED, coap_get_code_raw(&pkt));

tests/unittests/tests-nanocoap/tests-nanocoap.c
@@ -1109,7 +1109,7 @@ static void test_nanocoap__token_length_ext_269(void)
int res = coap_parse(&pkt, buf, 275);

TEST_ASSERT_EQUAL_INT(0, res);
TEST_ASSERT_EQUAL_INT(204, coap_get_code(&pkt));
TEST_ASSERT_EQUAL_INT(204, coap_get_code_decimal(&pkt));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_ASSERT_EQUAL_INT(204, coap_get_code_decimal(&pkt));
TEST_ASSERT_EQUAL_INT(COAP_CODE_CHANGED, coap_get_code_raw(&pkt));

@benpicco
Copy link
Contributor Author

benpicco commented Oct 20, 2023 •
edited
Loading

Can't we just remove it?

I think it's still nicer to see e.g. 204 in debug output instead of 68 when we get COAP_CODE_CHANGED.

Having it as a convenience function for that (with an explicit name) does no harm.

@riot-ci
Copy link

riot-ci commented Oct 20, 2023 •
edited
Loading

Murdock results

✔️ PASSED

e2e5e03 nanocoap_sock: use coap_get_code_raw()

Success Failures Total Runtime
7957 0 7957 10m:42s

Artifacts

@bergzand
Copy link
Member

Having it as a convenience function for that (with an explicit name) does no harm.

I agree with this use case. I think it does make sense to give it a name that makes it a bit more clear that it is only meant for human readable output.

@miri64
Copy link
Member

miri64 commented Oct 26, 2023

If it is just about convenience when debugging, why not have it output a string directly? Something with the signiture, e.g.,

static inline char *coap_get_code(const coap_pkt_t *pkt, char *str);

that you can just use it in a printf();

@benpicco
Copy link
Contributor Author

Why arbitrarily limit the possible use of the function like that?

@miri64
Copy link
Member

miri64 commented Oct 26, 2023

Because what else would you want to do with a function that behaves wrong?

@benpicco
Copy link
Contributor Author

It's not behaving wrong, it just has the wrong name. coap_get_code() makes it sound like it's the canonical way to get the CoAP code.
There is nothing wrong with a function that returns the decimal representation of the CoAP code if the name makes it clear what it does.

@miri64
Copy link
Member

miri64 commented Oct 26, 2023

There is nothing wrong with a function that returns the decimal representation of the CoAP code if the name makes it clear what it does.

Isn't it missing the decimal point for the decimal representation ;-)?

@benpicco benpicco mentioned this pull request Oct 27, 2023
Merged
@benpicco benpicco requested a review from maribu November 27, 2023 16:43
maribu
maribu approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Let's get this in as is, as this beyond any doubt improves the status quo.

I do agree that 2.05 would be the right string and preferred over 205. But we can continue the discussion after this is merged just as well and in the meantime have an improvement upstream.

@benpicco benpicco force-pushed the coap_get_code_decimal branch from 7a425c2 to e2e5e03 Compare November 28, 2023 12:11
@benpicco benpicco added this pull request to the merge queue Nov 28, 2023
Merged via the queue into RIOT-OS:master with commit a36817c Nov 28, 2023
@benpicco benpicco deleted the coap_get_code_decimal branch November 28, 2023 14:55
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@chrysn chrysn chrysn left review comments

@maribu maribu maribu approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2024.01
Development

Successfully merging this pull request may close these issues.
7 participants
@benpicco @riot-ci @bergzand @miri64 @chrysn @maribu @MrKevinWeiss