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: Remove API's empty default implemetations #12293

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

mirelachirica
Copy link
Contributor

@mirelachirica mirelachirica commented Jan 21, 2020

Summary of changes

Cleanup the Cellular API by removing empty default implementations and making them pure virtual.

Targeted for release-version 6.0.0.

Impact of changes

  • Classes inheriting CellularDevice will have to have implementation for the new pure virtual methods:

    • clear()
    • get_context_list()

  • Classes inheriting CellularContext do not have to have implementation for:

    • get_netmask()
    • get_gateway()

    if they do not differ from the NetworkInterface's default implemetations:
    nsapi_error_t NetworkInterface::get_netmask(SocketAddress *)
    {
        return NSAPI_ERROR_UNSUPPORTED;
    }
    
    nsapi_error_t NetworkInterface::get_gateway(SocketAddress *)
    {
        return NSAPI_ERROR_UNSUPPORTED;
    }
    

Migration actions required

See Impact of changes section.

Documentation

None


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[x] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@ciarmcom ciarmcom requested review from a team January 21, 2020 14:00
@ciarmcom
Copy link
Member

@mirelachirica, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

This is a breaking change, release notes are missing.

@adbridge
Copy link
Contributor

@mirelachirica looks like you removed the impact of changes and migration sections from the template. Please re-add them and fill them in as per the requirements for a breaking change - the PR type should also be changed as this is not a patch. Thanks.

@adbridge
Copy link
Contributor

@mirelachirica Shouldn't this be a feature type PR as you are changing APIs ?

@adbridge
Copy link
Contributor

This is a breaking change, release notes are missing.

Actually as a breaking change it should go as Major

@mirelachirica
Copy link
Contributor Author

This is a breaking change, release notes are missing.

Actually as a breaking change it should go as Major

Yes. Done.

@mirelachirica
Copy link
Contributor Author

Can this proceed? Does it need more reviews?

@mirelachirica
Copy link
Contributor Author

@AnttiKauppila could you review this?

@0xc0170 0xc0170 requested a review from bulislaw February 5, 2020 16:33
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 5, 2020

CI started

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 5, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 5, 2020

@mirelachirica Can you add details about this breaking change to the commit itself ? it's important commit by itself. While we have release notes, commit message is also crucial to contain at least reasoning for this change and what it means to a user when they update.

@0xc0170 0xc0170 dismissed bulislaw’s stale review February 5, 2020 16:38

Release notes added

@mbed-ci
Copy link

mbed-ci commented Feb 5, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 8dc15ee into ARMmbed:master Feb 6, 2020
@mergify mergify bot removed the ready for merge label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING-CHANGE release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants