coap: Replace `COAP_CT_` with `COAP_FORMAT_` #10108

bergzand · 2018-10-03T19:35:03Z

Contribution description

According to the IANA defines, there is only CoAP content-formats. and no CoAP content types. This PR merges the list of content types into the list of content formats and replaces all COAP_CT_.*occurences in our code with COAP_FORMAT_.*. I merged into the COAP_FORMAT_ options because those were more often used than the COAP_CT_ defines.

Not sure whether to call this an API_CHANGE, but all example code uses COAP_FORMAT_, so the old defines "should" not have been used outside our implementations, but if somebody wants to call this an api change I'm fine with it.

Testing procedure

A small test to verify that the .well-known/core still has content number 40 should be enough here.

Issues/PRs references

None

smlng

(minimal) tested ACK

smlng · 2018-10-08T08:10:15Z

one thing: isn't this somehow an API CHANGE? Some RIOT external APP could use the old defines, right?

bergzand · 2018-10-08T09:12:21Z

one thing: isn't this somehow an API CHANGE? Some RIOT external APP could use the old defines, right?

Yeah, that's why I mentioned it in the opening post. If everybody nicely based their applications on our examples it should not be a problem, but that's a huge if there. Anyway, I'm fine with this being considered an API change.

smlng · 2018-10-08T09:16:46Z

I guess it's good to have the label for documentation and release changelog creation.

smlng · 2018-10-08T09:17:15Z

though I'd like to have second ACK on this one then, too

bergzand · 2018-10-08T09:18:25Z

I guess it's good to have the label for documentation and release changelog creation.

Fair enough

though I'd like to have second ACK on this one then, too

Agreed

Thanks for the review!

kb2ma

Thanks for fixing this issue, @bergzand! The COAP_CT_xxx constants predate the merge of nanocoap and gcoap, and I agree it's time to disable these. I also agree it's worthwhile to consider existing users of the COAP_CT macros, although I expect there are few of them.

So, does it make sense to mark the COAP_CT_xxx macros as deprecated for a couple of releases? I would like to see us get into this mechanism for nanocoap/gcoap as the API will be evolving for function names. For example, coap_put_option_ct() (with the same problem as these macros) should change to coap_opt_put_format() or something like that. So, it makes sense to me to deprecate the current function name for a while.

Besides that, this work tests fine for me.

jia200x · 2018-10-10T14:11:52Z

So, does it make sense to mark the COAP_CT_xxx macros as deprecated for a couple of releases? I would like to see us get into this mechanism for nanocoap/gcoap as the API will be evolving for function names. For example, coap_put_option_ct() (with the same problem as these macros) should change to coap_opt_put_format() or something like that. So, it makes sense to me to deprecate the current function name for a while.

I'm +1 for adding a @deprecated tag to these macros

bergzand · 2018-10-13T22:27:31Z

Restored the removed defines and added a @deprecated tag with a message to them.

kb2ma · 2018-10-14T13:35:48Z

Thanks, @bergzand. The deprecated tag really stands out in Doxygen. This all looks good and tests fine for me.

There are a few uses of the "deprecated" tag in the code base. I don't know about historical use though. So, I would like to merge this PR, and then I plan to send create an RFC Issue to confirm there is no concern with this approach systematically for evolving the gcoap/nanocoap APIs forward. It helps to have an example. :-)

@smlng, any concerns? If not, I'm ready to squash and go.

sys/include/net/coap.h

kb2ma · 2018-10-14T13:52:02Z

Link is an improvement (thanks @miri64), and bold looks even better. Unfortunately the use of black color for text means it doesn't stand out as a link.

bergzand · 2018-10-14T14:06:08Z

Unfortunately the use of black color for text means it doesn't stand out as a link.

Yeah, also noticed that, not sure if we can fix that without modifying the css file.

smlng · 2018-10-15T07:32:23Z

Looks good to me, please squash

Replaces all occurences of COAP_CT_.* with COAP_FORMAT_.*

COAP_CT_ style defines for the content types are deprecated in favour of COAP_FORMAT_ style defines. COAP_FORMAT_ is expanded to include any missing content type that was available with COAP_CT_.

bergzand · 2018-10-15T07:45:48Z

Squashed and rebased. I've reworded the second commit message to better reflect the current changes.

smlng · 2018-10-15T09:25:22Z

@bergzand you broke the core 😉 I'll rerun Murdock

kb2ma · 2018-10-15T09:29:23Z

I saw the build failure. What went wrong?

Also, and this is drifting off topic, but when is it appropriate to tag with 'run tests'? I looked around yesterday for more background, but could not find an answer.

bergzand · 2018-10-15T09:32:57Z

@bergzand you broke the core wink I'll rerun Murdock

But how...?

smlng · 2018-10-15T09:37:52Z

@bergzand I was joking, I think the test is flawed or at least not deterministic in results on native

kb2ma · 2018-10-15T09:48:36Z

Thanks everyone for talking this through and improvements.

bergzand · 2018-10-15T09:49:32Z

Now we only have to remember to actually remove the defines in a release or two :)

miri64 · 2018-10-15T09:54:32Z

Now we only have to remember to actually remove the defines in a release or two :)

At some point we need to go through this list anyway and start removing stuff ;-)

jia200x · 2018-10-15T10:12:39Z

Great! Thanks so much to everyone involved!

At some point we need to go through this list anyway and start removing stuff ;-)

+1. There could be some kind of automation between releases :)

miri64 · 2018-10-15T10:25:51Z

I think letting some bot remove stuff from our code is something that would lead to desaster.

miri64 · 2018-10-15T10:28:23Z

(also the interval deprecated should be removed is something that should very much be decided on a case-by-case basis)

kb2ma · 2018-10-15T11:03:32Z

Thanks all for the comments! I plan to post a deprecation RFC in the next few days, and will look for consensus on a trial approach to this workflow.

BTW @miri64, I know I have seen that "Deprecated List" in Doxygen hundreds of times as I click into something else, but it didn't register in my mind that it existed when I looked for existing deprecated code. Thanks for calling that out.

jia200x · 2018-10-15T11:12:33Z

I think letting some bot remove stuff from our code is something that would lead to desaster.

I mean something to warn that a feature should be deprecated. We could tag the time somehow.
Never intended to automatically remove tags but more into the direction of inform maintainers :)

bergzand requested review from haukepetersen, kaspar030 and kb2ma October 3, 2018 19:35

jia200x added this to the Release 2018.10 milestone Oct 4, 2018

smlng approved these changes Oct 8, 2018

View reviewed changes

smlng added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Oct 8, 2018

kb2ma reviewed Oct 8, 2018

View reviewed changes

kb2ma approved these changes Oct 14, 2018

View reviewed changes

miri64 reviewed Oct 14, 2018

View reviewed changes

sys/include/net/coap.h Show resolved Hide resolved

bergzand force-pushed the pr/coap/merge_ct_format branch from f4547c5 to 43bdf4e Compare October 15, 2018 07:38

bergzand force-pushed the pr/coap/merge_ct_format branch from 43bdf4e to d76cf06 Compare October 15, 2018 07:40

bergzand added 2 commits October 15, 2018 09:44

coap: use COAP_FORMAT style content-format defines

726ebf3

Replaces all occurences of COAP_CT_.* with COAP_FORMAT_.*

coap: deprecate COAP_CT_ content type defines

8ee2bc5

COAP_CT_ style defines for the content types are deprecated in favour of COAP_FORMAT_ style defines. COAP_FORMAT_ is expanded to include any missing content type that was available with COAP_CT_.

bergzand force-pushed the pr/coap/merge_ct_format branch from d76cf06 to 8ee2bc5 Compare October 15, 2018 07:44

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 15, 2018

kb2ma merged commit cec477a into RIOT-OS:master Oct 15, 2018

bergzand deleted the pr/coap/merge_ct_format branch October 15, 2018 09:49

kb2ma mentioned this pull request Sep 28, 2019

net/coap: remove deprecated COAP_CT_... defines #12321

Merged

coap: Replace COAP_CT_ with COAP_FORMAT_ #10108

coap: Replace COAP_CT_ with COAP_FORMAT_ #10108

Uh oh!

Conversation

bergzand commented Oct 3, 2018

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Oct 8, 2018

Uh oh!

bergzand commented Oct 8, 2018

Uh oh!

smlng commented Oct 8, 2018

Uh oh!

smlng commented Oct 8, 2018

Uh oh!

bergzand commented Oct 8, 2018

Uh oh!

kb2ma left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Oct 10, 2018

Uh oh!

bergzand commented Oct 13, 2018

Uh oh!

kb2ma commented Oct 14, 2018

Uh oh!

Uh oh!

kb2ma commented Oct 14, 2018

Uh oh!

bergzand commented Oct 14, 2018

Uh oh!

smlng commented Oct 15, 2018

Uh oh!

bergzand commented Oct 15, 2018

Uh oh!

smlng commented Oct 15, 2018

Uh oh!

kb2ma commented Oct 15, 2018

Uh oh!

bergzand commented Oct 15, 2018

Uh oh!

smlng commented Oct 15, 2018

Uh oh!

kb2ma commented Oct 15, 2018

Uh oh!

bergzand commented Oct 15, 2018

Uh oh!

miri64 commented Oct 15, 2018

Uh oh!

jia200x commented Oct 15, 2018

Uh oh!

miri64 commented Oct 15, 2018

Uh oh!

miri64 commented Oct 15, 2018

Uh oh!

kb2ma commented Oct 15, 2018

Uh oh!

jia200x commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coap: Replace `COAP_CT_` with `COAP_FORMAT_` #10108

coap: Replace `COAP_CT_` with `COAP_FORMAT_` #10108