Skip to content

[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

Merged
merged 22 commits into from
Aug 26, 2016

Conversation

itaiferber
Copy link
Contributor

@itaiferber itaiferber commented Aug 24, 2016

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.)

@parkera
Copy link
Contributor

parkera commented Aug 24, 2016

Looks great!

@itaiferber
Copy link
Contributor Author

@parkera Thanks. :)
I originally had an additional column in each table for what file every entity was defined in (for easy lookup, and to note that, say, NSDateInterval is actually defined in NSDate.swift), but the tables are already pretty wide... Not sure if that's information we care to preserve, but it's an option.

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 |

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 not require a concrete implementation remain unimplemented

or

Methods which do not require a concrete implementation need updating to reflect this.

Copy link
Contributor Author

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.

@itaiferber itaiferber changed the title Update status document with test coverage information [WIP] Update status document with test coverage information Aug 25, 2016
@itaiferber
Copy link
Contributor Author

@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

@itaiferber
Copy link
Contributor Author

@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

@parkera
Copy link
Contributor

parkera commented Aug 26, 2016

LGTM

@itaiferber itaiferber merged commit 03995a2 into master Aug 26, 2016
@itaiferber itaiferber deleted the pr-test-coverage branch August 26, 2016 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants