Skip to content

api->API #8461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 30, 2018
Merged

api->API #8461

merged 3 commits into from
Oct 30, 2018

Conversation

kegilbert
Copy link
Contributor

Description

Minor doxygen update, uppercase acronym

Pull request type

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

@kegilbert
api->API
7709b24 
Minor doxygen update, uppercase acronym
Copy edit FileHandle.h
3a8919b 
Copy edit file.
AnotherButler
AnotherButler reviewed Oct 17, 2018
View reviewed changes
Copy link
Contributor

@AnotherButler AnotherButler left a comment

Choose a reason for hiding this comment

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

Please address my comments.

platform/FileHandle.h Outdated
@@ -207,7 +207,7 @@ class FileHandle : private NonCopyable<FileHandle> {
/** Check for poll event flags
* The input parameter can be used or ignored - the could always return all events,
Copy link
Contributor

Choose a reason for hiding this comment

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

Query: What should we replace "the" with in this sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AnotherButler : Good catch, this is saying the caller can either pass in a filter for the events they want, or filter the return value and not use the argument depending on how this virtual function is implemented. I think that the was meant to be a they (meaning whoever is implementing this function).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could I rewrite this sentence as "You can use or ignore the input parameter. You can return all events or check just the events listed in events."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds fine to me.

platform/FileHandle.h Outdated
* Note! This is not intended as an attach-like asynchronous api, but rather
* as a building block for constructing such functionality.
* Note! This is not intended as an attach-like asynchronous API, but rather
* as a building block for constructing such functionality.
*
* The exact timing of when the registered function
* is called is not guaranteed and susceptible to change. It should be used
Copy link
Contributor

Choose a reason for hiding this comment

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

Query: Is this saying that the registered function is not susceptible to change, or it is susceptible change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geky

From my understanding this is saying this function alone is not sufficient for an async format but can be used to develop one.

Copy link
Contributor

Choose a reason for hiding this comment

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

The timing of this function may change from release to release. Does that make sense?

@cmonr
Copy link
Contributor

cmonr commented Oct 19, 2018

@AnotherButler Is this waiting on you to resolve the queries, or @kegilbert to answer them?

Or @cmonr / @ARMmbed/mbed-os-maintainers / @ARMmbed/mbed-os-docs to provide some sort of answer?

@kegilbert
Copy link
Contributor Author

@cmonr @AnotherButler Sorry about that, my page wasn't loading properly when I submitted my comments before, they should be up now.

@0xc0170 0xc0170 closed this Oct 22, 2018
@0xc0170 0xc0170 deleted the kegilbert-patch-6 branch October 22, 2018 13:53
@AnotherButler
Copy link
Contributor

Why did we close this?

@kegilbert
Copy link
Contributor Author

kegilbert commented Oct 22, 2018 •
edited
Loading

Ah, think it hit the 3 day mark due to the weekend. @0xc0170 I'm going to reopen this if that's alright.

@kegilbert kegilbert restored the kegilbert-patch-6 branch October 22, 2018 16:09
@kegilbert kegilbert reopened this Oct 22, 2018
@cmonr
Copy link
Contributor

cmonr commented Oct 22, 2018

It was closed because we were doing branch cleanup.

Someone opened a PR using ARMmbed as the user.

@cmonr
Copy link
Contributor

cmonr commented Oct 23, 2018

A reminder that this still needs work.

Would be nice if this could be rolled up with the other docs PRs.

@cmonr cmonr removed the rollup PR label Oct 24, 2018
@cmonr
Copy link
Contributor

cmonr commented Oct 29, 2018

@kegilbert ^^^

@kegilbert
Copy link
Contributor Author

@cmonr This should be good to go.

@cmonr cmonr mentioned this pull request Oct 30, 2018
Merged
@0xc0170 0xc0170 merged commit 9ab13df into master Oct 30, 2018
@0xc0170 0xc0170 removed the needs: CI label Oct 30, 2018
@cmonr cmonr removed the rollup PR label Oct 30, 2018
@AnotherButler AnotherButler deleted the kegilbert-patch-6 branch October 30, 2018 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geky geky geky left review comments

@AnotherButler AnotherButler AnotherButler approved these changes

@cmonr cmonr Awaiting requested review from cmonr

Assignees
No one assigned
Labels
release-version: 5.10.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kegilbert @cmonr @AnotherButler @geky @0xc0170