-
Notifications
You must be signed in to change notification settings - Fork 325
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
fixes #54 support for draft V6, V7 and V2019-09 #213
Conversation
The changes look really good for the most part. The only other thing I can think to comment on is the fact that the draft 4 SpecVersion is hard coded into a lot of the tests. Maybe you could use Junit's Parameterized test runner. (I use TestNG more than Junit, so sorry if I'm not sure how to use this correctly.) It looks like the Parameterized test runner would allow the tests to be executed for each SpecVersion. That's just a suggestion in order to improve the tests, but it might not be necessary. |
I haven't thought about the test cases in detail yet. Hardcoded V4 is to ensure that all the changes are backward compatible so that it won't impact the current users. If there is a way that we can run the test against all spec factory, it will be great. I am not familiar with TestNG, would you please try something? Thanks. |
Codecov Report
@@ Coverage Diff @@
## master #213 +/- ##
============================================
+ Coverage 70.12% 70.79% +0.66%
- Complexity 520 567 +47
============================================
Files 62 68 +6
Lines 2032 2236 +204
Branches 441 482 +41
============================================
+ Hits 1425 1583 +158
- Misses 419 457 +38
- Partials 188 196 +8
Continue to review full report at Codecov.
|
I don't know if I should commit to your pull request. I was always taught not to commit to other people's branches at my workplace. I'm just going to show an example of what I'm talking about and I can add a commit if you really want me to.
|
@jawaff I don't mind if you check into the same branch and this might be the most efficient way to work together. However, if you don't feel comfortable, could you please approve the PR and you can start another one after the merge. Thanks. |
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.
@stevehu, I liked the way you handle which commands are implemented for each schema using the position of bits. Thanks for the documentation and all your effort. This change will make it even easier to add new commands.
@andersonf @jawaff Thanks a lot for the reviews and feedbacks. I have merged this PR. Please submit the PRs for the test runner and if-else-then. Contribution credits need to go to the contributors :) |
@stevehu This was such a great change. Thanks for the effort, Steve. I'll look into making a pull request later today or tomorrow. |
@jawaff I was inspired by the ideas you suggested. |
I have spent several hours today to refactor the library so that we can support multiple specification versions easily for both developers and end-users. @jawaff and I have discussed the design for several days in the #206 and we finally decide to implement it this way.
I also added a document in the doc folder named specversion.md for both users and developers.