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: Chainstack implementation update #6

Merged
merged 12 commits into from
Aug 4, 2022

Conversation

soos3d
Copy link
Contributor

@soos3d soos3d commented Aug 2, 2022

What I did

Checklist

  • [x ] Passes all linting checks (pre-commit and CI jobs)
  • [ x] New test cases have been added and are passing
  • [ x] Documentation has been updated
  • [ x] PR title follows Conventional Commit standard (will be automatically included in the changelog)

soos3d added 3 commits August 2, 2022 15:19
Update README with clear instructions
Remove Kovan as it is not supported by Chainstack
@soos3d
Copy link
Contributor Author

soos3d commented Aug 2, 2022

This time should not have conflict as I re-forked the repo with your updates (I only removed Kovan because is not supported by Chainstack)

Copy link
Contributor

@NotPeopling2day NotPeopling2day left a comment

Choose a reason for hiding this comment

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

Looks good! Quick recommendation is to change reference to the Ape Framework vs ApeWorX the organization.

soos3d and others added 6 commits August 4, 2022 17:56
Co-authored-by: NotPeopling2day <[email protected]>
Co-authored-by: NotPeopling2day <[email protected]>
Co-authored-by: NotPeopling2day <[email protected]>
Co-authored-by: NotPeopling2day <[email protected]>
Co-authored-by: NotPeopling2day <[email protected]>
Co-authored-by: NotPeopling2day <[email protected]>
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

Looking good!
Consider my feedback on the networks list thought

README.md Outdated
```

This is a list and syntax of the networks available once you install the Chainstack plugin.

```bash
Copy link
Member

Choose a reason for hiding this comment

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

Consider an explanation instead of enumerating all the options here. Something like:

You compose the network option by following this formula: <ecosystem>:<network>:<provider> where "chainstack" is the provider.
See the full output of networks in your ape instance by using the command `ape networks list`.
For more information on networking un ape, see [this guide (https://docs.apeworx.io/ape/stable/userguides/networks.html).

Let's say we increase support for new networks in chainstack; this list will become outdated. I am trying to avoid someone having to keep this documentation up to date as the plugin grows.

Copy link
Member

Choose a reason for hiding this comment

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

Linking to this doc is probably sufficient https://docs.apeworx.io/ape/stable/userguides/networks.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, you can go ahead and propose the change if you can, it's probably quicker like that than if I change it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually updated it in on my side. What is a good way to implement it here now?

soos3d and others added 3 commits August 4, 2022 18:05
Co-authored-by: Juliya Smith <[email protected]>
Co-authored-by: Juliya Smith <[email protected]>
Copy link
Contributor

@NotPeopling2day NotPeopling2day left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@NotPeopling2day NotPeopling2day merged commit 615860b into ApeWorX:main Aug 4, 2022
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.

3 participants