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

Cellular: unified return value comments on API folder. #8814

Merged
merged 1 commit into from
Nov 24, 2018

Conversation

jarvte
Copy link
Contributor

@jarvte jarvte commented Nov 20, 2018

Description

_ Fixed cellular API folder to follow same syntax when defining doxygen return values.
-CellularInformation had wrong return type (which is correct on deriving classes), fixed to correct return value. Does this make this Breaking change? Changed from nsapi_size_or_error_t to nsapi_error_t. Function implementation has returned only error codes.

@mirelachirica please review

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@jarvte
Copy link
Contributor Author

jarvte commented Nov 20, 2018

travis-ci/events failed, not related to this pr.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2018

Does this make this Breaking change? Changed from nsapi_size_or_error_t to nsapi_error_t. Function implementation has returned only error codes.

I would assume yes, in features/cellular/framework/API/CellularInformation.h. How does this affect a user ?

I restarted the events job

@jarvte
Copy link
Contributor Author

jarvte commented Nov 20, 2018

I would assume yes, in features/cellular/framework/API/CellularInformation.h. How does this affect a user ?

It should not affect as implementation has not changed, it has never returned any size/length. Only way it affects is that application should expect nsapi_error_t instead of nsapi_size_or_error_t. Both are typedefs to signed int. I'll change to breaking change,.

@AnttiKauppila
Copy link

This is a breaking change because function prototype is modified.

@bulislaw
Copy link
Member

@AnttiKauppila we are committed to maintaining backward compatibility. Is this necessary?

@kjbracey
Copy link
Contributor

Even if those were different scalar types, I'd struggle to call this a "breaking change", but in this case they're both typedefs to the same thing! It's reformatting!

@AnttiKauppila
Copy link

I was wrong, I just quickly read the code. I checked and both return values are signed int's so it is not a breaking change after all!

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 23, 2018

PR type updated accordingly.

@bulislaw This should be ready for CI now, labeling as such

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 23, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 24, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 1
Build artifacts
Build logs

@0xc0170 0xc0170 merged commit 5086c3d into ARMmbed:master Nov 24, 2018
@jarvte jarvte deleted the unify_cellular_return branch December 3, 2018 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants