Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

net/gcoap: deprecate gcoap_finish() #12838

Merged
merged 2 commits into from
Jan 14, 2020

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Nov 28, 2019

Contribution description

gcoap's implementation has been transitioning to use a common API with nanocoap to build a message:

This PR deprecates gcoap_finish(), with removal scheduled for after the 2020.04 release. This PR also deprecates the three GCOAP_xxx_OPTIONS_BUF macros, which are used only to accommodate gcoap_finish().

Also, somehow a use of gcoap_finish() has crept into the code in cord_epsim. I plan to create a separate PR to replace that use.

Testing procedure

Build the documentation and ensure deprecation notices appear for the function and the macros.

Issues/PRs references

See above.

@kb2ma kb2ma added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Nov 28, 2019
@kb2ma kb2ma added this to the Release 2020.01 milestone Nov 28, 2019
@kb2ma kb2ma requested a review from smlng November 28, 2019 17:51
@@ -739,6 +748,9 @@ int gcoap_req_init(coap_pkt_t *pdu, uint8_t *buf, size_t len,
* Assumes the PDU has been initialized with a gcoap_xxx_init() function, like
* gcoap_req_init().
*
* @deprecated Will be removed after the 2020.04 release. Use
Copy link
Member

Choose a reason for hiding this comment

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

Don't we usually remove after two cycles? We are in the middle of the 2020.01 release cycle, so the proper removal release would be 2020.07.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's what I was trying to say. When I read, "after the 2020.04 release", that implies sometime before 2020.07 is released. I adopted this expression from other deprecations I had seen. Perhaps I misunderstood.

IMO it would be clearer to say "Will be removed for the 2020.07 release." What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

IMO it would be clearer to say "Will be removed for the 2020.07 release." What do you think?

Yes, that is typically what we mean with "after two cycles": Starting from the current one under development + 2 releases. The "after" refers to the fact that it will be removed during the release development and thus not be available after the release is out.

Copy link
Member Author

@kb2ma kb2ma Dec 4, 2019

Choose a reason for hiding this comment

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

Just to be clear, I hear you say that you prefer:

Will be removed after the 2020.07 release.

rather than

Will be removed for the 2020.07 release.

To me "removed" is an active verb that means "the act of taking it out". So, it sounds like you are saying the act of taking out the function will happen after the release.

[edit -- scratch the text below; "after" just doesn't make sense to me in this context]
So, to keep the word "after" it would be clearer to use what you stated in your comment:

Will be unavailable after the 2020.07 release.

Copy link
Member

@miri64 miri64 Dec 4, 2019

Choose a reason for hiding this comment

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

You are the native speaker ^^" and what you say makes sense to me. Then let's go for "unavailable" or "not available [edit: anymore]" (the latter I prefer somehow, can't really explain why, though semantically they have the same meaning).

@kb2ma
Copy link
Member Author

kb2ma commented Dec 24, 2019

Comments addressed.

@fjmolinas
Copy link
Contributor

ping @miri64 !

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 13, 2020
@miri64
Copy link
Member

miri64 commented Jan 13, 2020

@kb2ma please squash!

@kb2ma kb2ma force-pushed the gcoap/deprecate_gcoap_finish branch from 04bcf81 to 57cd900 Compare January 13, 2020 15:19
@kb2ma
Copy link
Member Author

kb2ma commented Jan 13, 2020

Squashed and Travis happy.

@miri64
Copy link
Member

miri64 commented Jan 14, 2020

Murdock as well.

@miri64 miri64 merged commit 48221bb into RIOT-OS:master Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants