Skip to content
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

Merged
merged 16 commits into from
Feb 1, 2022
Merged

docs(x/data): update specification #673

merged 16 commits into from
Feb 1, 2022

Conversation

ryanchristo
Copy link
Member

@ryanchristo ryanchristo commented Dec 16, 2021

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

  • included the correct docs: prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the documentation writing guidelines
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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

  • confirmed the correct docs: prefix in the PR title
  • confirmed all author checklist items have been addressed
  • confirmed that this PR only changes documentation
  • reviewed content for consistency
  • reviewed content for thoroughness
  • reviewed content for spelling and grammar
  • tested instructions (if applicable)

@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #673 (a96a661) into master (63eb64d) will increase coverage by 0.06%.
The diff coverage is n/a.

❗ Current head a96a661 differs from pull request most recent head ef73af5. Consider uploading reports for the commit ef73af5 to get more accurate results

@@            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     
Flag Coverage Δ
experimental-codecov 74.90% <ø> (+0.06%) ⬆️
stable-codecov 67.65% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ryanchristo ryanchristo added the Scope: x/data Issues that update the x/data module label Dec 17, 2021
@ryanchristo ryanchristo requested a review from clevinson January 25, 2022 17:35
Copy link
Collaborator

@robert-zaremba robert-zaremba left a 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

@ryanchristo
Copy link
Member Author

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

@clevinson
Copy link
Member

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.

Copy link
Member

@clevinson clevinson left a 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).

@clevinson clevinson enabled auto-merge (squash) January 28, 2022 00:41
@ryanchristo
Copy link
Member Author

Thanks @clevinson. Opened #711 to further discuss and track updates to format.

auto-merge was automatically disabled January 28, 2022 20:04

Pull Request is not mergeable

x/data/spec/02_state.md Outdated Show resolved Hide resolved
x/data/spec/03_messages.md Outdated Show resolved Hide resolved
x/data/spec/05_client.md Outdated Show resolved Hide resolved
x/data/spec/06_tests.md Outdated Show resolved Hide resolved
x/data/spec/06_tests.md Show resolved Hide resolved
ryanchristo and others added 4 commits January 31, 2022 17:32
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>
@ryanchristo ryanchristo enabled auto-merge (squash) February 1, 2022 01:37
Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

@ryanchristo ryanchristo merged commit ca2b04f into master Feb 1, 2022
@ryanchristo ryanchristo deleted the ryan/571-data-spec branch February 1, 2022 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: x/data Issues that update the x/data module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Module documentation
5 participants