Implements unified result structure #221
Conversation
|
Is this WIP or ready to be reviewed? (I don't see the PR as draft PR or the in progress label. But I don't see reviewers assigned either and I do see failing tests, so I'm probably just too proactive 😅 ) |
|
Sorry, not used to draft feature. This is a WIP (I've attached the label now). You are always welcome to overview, but things may change. I would mainly focus on the |
artem-zakharchenko
force-pushed
the
unified-api
branch
5 times, most recently
from
7505169 to
f0a12e4
Compare
artem-zakharchenko
force-pushed
the
unified-api
branch
from
268048b to
50883ae
Compare
BREAKING CHANGE: - Gavel fields no longer contain the following properties: - `validator` - `expectedType` - `realType` - `rawData` - Gavel fields now contain the next new properies: - `kind` - `values` (optional)
artem-zakharchenko
force-pushed
the
unified-api
branch
from
afdec85 to
4b24fb6
Compare
|
For the sake of maintaining the scope of this pull request to a reasonable extent, I will rework the internal of certain validators (i.e. That is because Gavel treats all validators the same, as most of them inherit from the same base class ( As validators are scoped, I would like not to bring up their refactoring at the moment. |
artem-zakharchenko
force-pushed
the
unified-api
branch
from
6a41559 to
a95de94
Compare
|
a95de94 removes diff'ing from Also adjusted unit tests for TextDiff to match the changes. |
|
It seems I've misconfigured the exclusion of |
|
I've fixed the cli scenarios exclusion in Windows and the build pipeline is green now. I will look into remaining unit tests, but it looks unlikely to refactor them thoroughly without rewriting the validators (#150), which I would like to omit as a part of this pull request. There is a decent chance this pull request is ready for review. Will confirm explicitly today. |
There was a problem hiding this comment.
Awesome job! A lot of good work. I have just a few comments, mostly about leftovers or nitpicks you can even ignore.
Also, I wonder if we could put the Cucumber steps into one files as it's just a few of them now and I don't see much value in having them categorized as general.js or fields.js. But that can be done as a next step with upgrading Cucumber, feel free to ignore the suggestion now.
| } | ||
| ); | ||
|
|
||
| this.Then(/^exit status is (\d+)$/, function(expectedStatus) { | ||
| assert.equal(this.status, expectedStatus, 'Process statuses do not match'); | ||
| this.Then(/^exit status is (\d+)$/, function(expectedExitCode) { |
There was a problem hiding this comment.
Would be nice to change the wording also in the spec for consistency, but let's not bother with this now.
| @@ -59,7 +59,7 @@ module.exports = function() { | |||
| }); | |||
|
|
|||
| // Vocabulary proxy over the previous action for better scenarios readability. | |||
| this.When(/^I call "([^"]*)"$/, function(_command) { | |||
| this.When(/^I call "gavel.validate(([^"]*))"$/, function(_, _command) { | |||
There was a problem hiding this comment.
Shouldn't it be gavel.validate\(([^\)]*)\)? Not important if it works correctly like this
There was a problem hiding this comment.
It does seem to work right now, so we can improve it in #113 :)
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Contains implementation of the unified result structure and internals re-design for Gavel. This pull request introduces breaking changes.
GitHub
Roadmap
TextDiffperform a plain text comparison without any diffs