-
Notifications
You must be signed in to change notification settings - Fork 103
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
docs(x/data): update specification #673
Conversation
Codecov Report
@@ Coverage Diff @@
## master #673 +/- ##
==========================================
+ Coverage 74.74% 74.80% +0.06%
==========================================
Files 118 118
Lines 19542 19542
==========================================
+ Hits 14606 14618 +12
+ Misses 3963 3947 -16
- Partials 973 977 +4
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Similar to: #633 (review)
Let's simplify the acceptance tests
I don't think changing the language is necessary. I think we should follow the standard format for acceptance tests: https://openclassrooms.com/en/courses/4544611-write-agile-documentation-user-stories-acceptance-tests/4810081-write-acceptance-tests |
I'm actually not sure from reading this that the "if..then" is part of the standard format. When I read this it makes me think that the "if...then" statements they provide are only to help one generate a GIVEN...WHEN...THEN style acceptance test. I think these are all fine to leave in in the current UATs. In the future I do agree that they can be simpler (5-10 word) summaries of what the condition is. But I don't think this is really anything worth bike shedding over. |
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.
Thx @ryanchristo !
I noticed that the pages weren't actually visible in the table of contents, or in the Data Module overview page. So I added a few commits to fix that. Otherwise lgtm 🎉 !
I think its good for these to be visible even though they're not available on mainnet (and just added a note abt only being available in experimental builds in the data module overview page).
Thanks @clevinson. Opened #711 to further discuss and track updates to format. |
Pull Request is not mergeable
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
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.
👍🏻
Description
Closes: #571
This pull request updates the data module specification to reflect the latest version.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
docs:
prefix in the PR titleReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
docs:
prefix in the PR title