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

Flow.rs SDK Milestone 4 #79

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Flow.rs SDK Milestone 4 #79

merged 1 commit into from
Nov 2, 2021

Conversation

fee1-dead
Copy link
Contributor

@fee1-dead fee1-dead commented Oct 26, 2021

Description

This PR is for issue #20.

flow-sdk has met the requirements for Milestone 4.

Submission Links & Documents

See rendered, the flow.rs repository and REFERENCE.md.

flow-sdk now has 100% doc coverage.

Requirements Check

  • Have have you met the milestone requirements? YES
  • Have you included tests (if applicable)? YES
  • If this is the last milestone:
    • Demonstrate that you've met all the acceptance criteria (link to code, demos, instructions to run etc.)

    • Demonstrate that you've met all milestone requirements and highlight any extensions or additional work done.

See the flow.rs repository.

Other Details

  • Is there anything specific you'd like the PoC to know or review for?
  • Are there other references, documentation, or relevant artificats to mention for this PR (ie. external links to justify design decisions, etc.)?

None at the moment

@fee1-dead fee1-dead marked this pull request as ready for review October 27, 2021 15:06
@fee1-dead fee1-dead marked this pull request as draft October 27, 2021 15:06
@fee1-dead fee1-dead marked this pull request as ready for review October 29, 2021 05:47
@sideninja
Copy link
Contributor

Hi @fee1-dead thank you for the submission, this SDK is looking great.

One thing regarding documentation I've noticed is that it doesn't support API reference, do you still plan to add this or not? I would highly recommend adding it as it will keep documentation up to date. Also make sure you delete all TODOs from docs and maybe include the docs in /docs subfolder as README so it's opened by default in github once you navigate there, reference might not be the best location.

I feel like example for getting transaction could be more straight forward and just include the get transaction by id functionality instead of going through blocks and collections, although I think that is an interesting example I suggest you include those transactions as extra in docs and keep the basic examples basic so they are easy to understand for new users. Same for collections.

The link in the readme is broken: "To install Rust, visit the rustup website for information."

What is the difference between flow-examples and examples?

I'm assuming you still plan to add tests? I think it would be a good idea to at least provide some basic unit tests as part f the milestone 4 submission.

@fee1-dead
Copy link
Contributor Author

Hello @sideninja,

One thing regarding documentation I've noticed is that it doesn't support API reference

What do you mean by this? The documentation is available at https://docs.rs/flow-sdk/.

Also make sure you delete all TODOs from docs and maybe include the docs in /docs subfolder as README so it's opened by default in github once you navigate there, reference might not be the best location.

Got it, thanks!

The link in the readme is broken: "To install Rust, visit the rustup website for information."

Fixed.

I feel like example for getting transaction could be more straight forward and just include the get transaction by id functionality instead of going through blocks and collections, although I think that is an interesting example I suggest you include those transactions as extra in docs and keep the basic examples basic so they are easy to understand for new users. Same for collections.

Thanks for the feedback! I do plan to update the examples soon.

What is the difference between flow-examples and examples?

I built examples starting out, but I felt it got a bit complicated because I only used testnet. I plan to remove examples and move significant ones to flow-examples.

flow-examples is like a cli prompt. It runs an emulator on startup and you run examples as if you are issuing commands. It is way more elegant and friendly to beginners because I can ship binaries for flow-examples but not examples.

I'm assuming you still plan to add tests? I think it would be a good idea to at least provide some basic unit tests as part of the milestone 4 submission.

Yes I do. I will look at the Go SDK and adopt some of its tests as well as adding some doctests (in case you didn't know, Rust tests code blocks inside documentation by default).

@sideninja
Copy link
Contributor

Awesome, keep me posted on the changes, otherwise I think you are good to go.

@fee1-dead
Copy link
Contributor Author

fee1-dead commented Nov 1, 2021

Hello @sideninja:

  • I have removed all TODO items inside docs/README.md, and added more examples to flow-examples.

  • I added a few tests for Cadence JSON encoding and decoding.

  • I am unsure of what tests to add next. I am pretty confident that there aren't any major bugs affecting the library, given that all examples in flow-examples run perfectly. Could you take a look and assist me on this matter? Thanks.

@sideninja sideninja merged commit 09d9326 into onflow:main Nov 2, 2021
@fee1-dead fee1-dead deleted the milestone-4-rust branch November 4, 2021 17:52
@fee1-dead
Copy link
Contributor Author

Hello @sideninja, here are some changes after my last comment:

  • I am preparing to release 1.0.0 version of my library. That means I would make less breaking changes per release (or more releases per breaking change). I try to find breaking changes that I want to make now, so I don't have to bump to 2.0.0 later.

  • I added a formatter config to ensure consistent format of imports between files.

  • I have made it so that examples inside the reference (at docs/README.md) will be tested in CI. (src)

  • flow-examples binary are built for each CI run. They are a bit inaccessible right now, you have to go to the actions page, and click on the latest CI run, and scroll down to see the artifact.

@sideninja
Copy link
Contributor

sideninja commented Nov 5, 2021

Awesome, great work. I don't have any more comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants