Refactoring to ease contribution #46
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 174
💛 - Coveralls |
|
Something went wrong in my rebase, so tests are failing. Please hold on review |
mroderick
changed the title
mantoni
requested changes
Nov 7, 2018
mantoni
approved these changes
Nov 7, 2018
|
You can go ahead with the review, it seems the only failure was reduction in coverage. (I will add a few commits to fix this) |
|
Looks good to me. Especially nice that you took the time to write documentation for the rather complicated test helper 👍 |
mroderick
changed the title
added 20 commits
November 7, 2018 12:41
These would be exposed as `assert.isTrue.test`, and would return a Promise. Some research into the git history reveals that this was added for some interal purpose when referee was a major component of busterjs, and used to be called `.internal` sinonjs@6d33cbb This was never documented as part of the public API.
The test should verify the end message, not how many times format was called
This is not used by referee itself
added 25 commits
November 7, 2018 12:42
This will help detect when the API changes
mroderick
force-pushed
the
refactoring-2
branch
from
fe32f50
to
57bfda4
Compare
|
Thank you for your review. Brace yourself for more pull requests with test coverage and refactoring |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As stated earlier (#42 (comment)), this is the first PR refactoring referee to make it easier to contribute to.
There is still a ways to go, but this PR is already huge.
I've tried to make it easy to follow by breaking down changes into commits. It may still take a long time to complete the review.
Breaking changes to the public API
I've taken the opportunity to tidy up the public API, and have removed a number of properties that end users should not be using.
This will mean that there are a number of things that we don't need to provide end user documentation for, and won't need to support forever.
In my opinion, it is better to tidy up the API now, before
refereegets too much traction.Removals:
referee.assert[every assertion].test- this was an internal artefact frombusterreferee.assert.json.jsonParseExceptionMessage- not to be used by end usersreferee.assert.exception.*Message- not to be used by end usersreferee.assert.className.noClassNameMessage- not to be used by end usersreferee.assert.tagName.noTagNameMessage- not to be used by end usersreferee.prepareMessage- not to be used by end usersreferee.format- not to be used by end usersreferee.assert.match.exceptionMessage- not to be used by end usersreferee.isArrayLike- not to be used by end usersreferee.match- not to be used by end usersHow to verify
npm installnpm testChecklist for author
npm run lintpasses