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: readme update #916

Merged
merged 1 commit into from
Aug 15, 2023
Merged

docs: readme update #916

merged 1 commit into from
Aug 15, 2023

Conversation

nagmo-starkware
Copy link
Contributor

@nagmo-starkware nagmo-starkware commented Jul 26, 2023

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information


This change is Reviewable

@nagmo-starkware
Copy link
Contributor Author

nagmo-starkware commented Jul 26, 2023

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #916 (360ac6c) into main (1cfcd13) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #916      +/-   ##
==========================================
- Coverage   74.61%   74.58%   -0.04%     
==========================================
  Files          71       71              
  Lines        6111     6111              
  Branches     6111     6111              
==========================================
- Hits         4560     4558       -2     
- Misses        742      745       +3     
+ Partials      809      808       -1     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @nagmo-starkware)


README.md line 134 at r1 (raw file):

* V0_4_0
Assuming the node is exposed at `local-host:8080` one might send requests via curl with:
`curl --location 'localhost:8080/rpc/V0_3_0' --header 'Content-Type: application/json' --data '{"jsonrpc":"2.0","id":0,"method":"starknet_blockHashAndNumber"}'`

Put this in a code block and not code line (

text

instead of text


README.md line 134 at r1 (raw file):

* V0_4_0
Assuming the node is exposed at `local-host:8080` one might send requests via curl with:
`curl --location 'localhost:8080/rpc/V0_3_0' --header 'Content-Type: application/json' --data '{"jsonrpc":"2.0","id":0,"method":"starknet_blockHashAndNumber"}'`

Choose a method that receives one argument and add the argument to the example

Copy link
Contributor Author

@nagmo-starkware nagmo-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @ShahakShama)


README.md line 134 at r1 (raw file):

Previously, ShahakShama wrote…

Choose a method that receives one argument and add the argument to the example

I picked this on purpose. I looked for something as small as possible. my intention is not to show how to use curl or json rpc but to show the path to send requests to

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @nagmo-starkware)


README.md line 134 at r1 (raw file):

Previously, nagmo-starkware wrote…

I picked this on purpose. I looked for something as small as possible. my intention is not to show how to use curl or json rpc but to show the path to send requests to

If that's your purpose, then you can just say that you can query the node at the URL localhost:8080/rpc/V0_3_0

ShahakShama
ShahakShama previously approved these changes Jul 27, 2023
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @nagmo-starkware)


README.md line 133 at r2 (raw file):

* V0_3_0
* V0_4_0
Assuming the node is exposed at `local-host:8080` one might send requests via curl with:

local-host -> localhost

@nagmo-starkware nagmo-starkware force-pushed the add_v0.4.0_not_connected_to_endpoint branch from c32ecd5 to e8b5e03 Compare July 27, 2023 07:43
Base automatically changed from add_v0.4.0_not_connected_to_endpoint to main July 27, 2023 07:58
@nagmo-starkware nagmo-starkware dismissed ShahakShama’s stale review July 27, 2023 07:58

The base branch was changed.

@dan-starkware
Copy link
Collaborator

README.md line 132 at r2 (raw file):

Current supported versions are:
* V0_3_0
* V0_4_0

Consider:

  • Add a reference to the spec
  • No patch here

Code quote:

Current supported versions are:
* V0_3_0
* V0_4_0

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)

a discussion (no related file):
I think this PR should be merged only after we wire v4


@nagmo-starkware nagmo-starkware force-pushed the readme_update branch 2 times, most recently from 95a5119 to 75f0970 Compare July 30, 2023 08:25
ShahakShama
ShahakShama previously approved these changes Jul 30, 2023
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)

@nagmo-starkware nagmo-starkware force-pushed the readme_update branch 9 times, most recently from 903a039 to 2999c2c Compare August 14, 2023 08:40
Copy link
Contributor Author

@nagmo-starkware nagmo-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @ShahakShama)

a discussion (no related file):

Previously, dan-starkware wrote…

I think this PR should be merged only after we wire v4

Done.



README.md line 134 at r1 (raw file):

Previously, ShahakShama wrote…

If that's your purpose, then you can just say that you can query the node at the URL localhost:8080/rpc/V0_3_0

I think this is a good example. if you disagree let's talk

ShahakShama
ShahakShama previously approved these changes Aug 14, 2023
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware)

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware)


README.md line 136 at r5 (raw file):

See spec at: [starknet-specs repo](https://github.com/starkware-libs/starknet-specs/)  

Suggestion:

See specification at: [starknet-specs repo](https://github.com/starkware-libs/starknet-specs/).

README.md line 140 at r5 (raw file):

```bash
curl --location 'localhost:8080/rpc/v0_3' --header 'Content-Type: application/json' --data '{"jsonrpc":"2.0","id":0,"method":"starknet_blockHashAndNumber"}'

Consider breaking the code to multiple lines so reader won't have to scroll.

Code quote:

Assuming the node is exposed at `localhost:8080` one might send requests via curl with:
```bash
curl --location 'localhost:8080/rpc/v0_3' --header 'Content-Type: application/json' --data '{"jsonrpc":"2.0","id":0,"method":"starknet_blockHashAndNumber"}'
___
*[`README.md` line 178 at r5](https://reviewable.io/reviews/starkware-libs/papyrus/916#-NbookT6AMziXZgqNkPV:-NbookT6AMziXZgqNkPW:bt8ak6t) ([raw file](https://github.com/starkware-libs/papyrus/blob/083a4b539096bbbeb5eabfb4c58fbec0ed0d66d3/README.md#L178)):*
> ```Markdown
> ## Deployment
> We provide a helm chart for deploying the node to a kubernetes cluster.
> It is located under the deployments folder.
> ```

Consider something like, if a folder doesn't work you can use the README



_Suggestion:_
```Markdown
[deployments folder](deployments/helm)

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware)

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nagmo-starkware)

@nagmo-starkware nagmo-starkware added this pull request to the merge queue Aug 15, 2023
Merged via the queue into main with commit 6356abf Aug 15, 2023
17 checks passed
@nagmo-starkware nagmo-starkware deleted the readme_update branch August 15, 2023 07:52
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants