[riot-notifications] [RIOT-OS/RIOT] API change, uart input not working anymore on previously working setups (#11525)

Gaëtan Harter notifications at github.com
Tue May 14 14:35:18 CEST 2019

It is a RIOT code base API change. If this is not something we describe as an API change, please let me know.

What is wrong here, is not that it needed to be changed, but that it was changed like this.

Touching low level dependencies with uart is sure tricky and can have consequences. The pull request does not have a testing procedure for this change… and does not give a testing procedure for showing it indeed solves the target issue.

The API change can be seen as it done in a single commit, where the shell dependencies must be updated as the same time to not be in a broken state.
If there was no API change, it could and should have been at least two commits.

It is a single line commit message "sys: make uart_stdio RX optional".
No more details on consequences and why.
Why does a commit named like this changes the `shell` dependencies without explanation?

No mention that anything that implicitly depended on this before may break.

All this minimal information led to this PR being handled as a minor change when it should not have been.

To prevent this, I think that we should move in a direction that contributor *should* put more information in the first place, and reviewer *should* ask for more information when there is not enough.
Even if both understand together the issue, another reader, now or in the future, may find this completely obscure.

> I guess we are narrowing down our options then.

The other solution is to now revert this and handle it with more time and mark it with an API change warning if needed.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190514/61f85a6a/attachment.html>

More information about the notifications mailing list