-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[WIP] Update status document with test coverage information #592
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
Conversation
Looks great! |
@parkera Thanks. :) In any case, I'll be updating this over the course of the next day or so so we hopefully get a better idea of where we're really lacking. |
| `PortMessage` | Unimplemented | ? | | | ||
| `RunLoop` | Mostly Complete | ? | `add(_: Port, forMode:)` and `remove(_: Port, forMode:)` remain unimplemented | | ||
| `NSStream` | Mostly Complete | ? | | | ||
| `Stream` | Unimplemented | ? | Methods which do not require a concrete implementation remain unimplemented | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence left me a bit confused. I was wondering if you meant one of the following?
Methods which
do notrequire a concrete implementation remain unimplemented
or
Methods which do not require a concrete implementation need updating to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye, thanks for catching this. I'd meant the first one — all methods which are not implemented in terms of NSRequiresConcreteImplementation()
have been left NSUnimplemented()
, and need implementations. I will push a correction.
…coverage information
7c81dad
to
b3db45d
Compare
@parkera When you get the chance, can you please review the changes I've added to our guidelines? I want to make sure the public-facing text is appropriate |
@parkera @phausler @michael-lehew Also, if you guys wouldn't mind sanity-checking the latest revision, I would appreciate it I want to make sure that everything seems reasonable before merging |
LGTM |
This brings
Docs/Status.md
up to date with the current state of implementation as part of an effort to improve documentation and test coverage across the project. With up-to-date information, it will be easier to see what components of our project require additional work, and more importantly, where we can improve in testing.This PR is not ready to merge as I will be pushing additional commits showing the actual state of test coverage, but I wanted to get this up in case anyone had comments or concerns with this initiative. I also intend to update our contribution guidelines to highlight that, ideally, PRs and changes to our codebase should also come with update to sections in the corresponding tables to reflect coverage and completeness.
(I recommend viewing the document to see what the proposed table format looks like.)