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

Update README #96

Merged
merged 17 commits into from
May 6, 2022
Merged

Update README #96

merged 17 commits into from
May 6, 2022

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Apr 11, 2022

Updates the README. Removed manual steps, but provided some necessary info (where to find id & cmds) to start it manually, in case it's necessary.

closes #92

@haerdib haerdib self-assigned this Apr 11, 2022
@haerdib haerdib requested a review from echevrier April 11, 2022 12:49
Copy link
Contributor

@echevrier echevrier 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 very much. It is much better. The commands are valid and working. But I am missing some info, in case something goes wrong.
The references to the substrate documentation are good, but it is unpleasant that in order to ensure that the different steps work as they should, you always have to refer to them.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@haerdib haerdib requested a review from echevrier April 12, 2022 11:06
@haerdib
Copy link
Contributor Author

haerdib commented Apr 12, 2022

I'm not explaining the things too in depth, because I don't think this is a tutorial for beginners. There are tutorials from substrate to cover the basics, such as the meaning of the different ports and similar. This is a README for developers that know how starting a parachain works, but don't necessarily know all commands by heart yet.

So as long as the commands in the README work, that is good enough. Covering all necessary debugging information would require a lot more work. Which might get outdated quite soon again.

Copy link
Contributor

@echevrier echevrier left a comment

Choose a reason for hiding this comment

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

Thanks.

@haerdib haerdib requested a review from brenzi April 13, 2022 13:15
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@haerdib haerdib requested a review from brenzi April 29, 2022 08:51
README.md Outdated
```
depending on how you installed `polkadot-launch`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer if we set a team standard here. Go with the second option but explain how to install it that way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it accordingly. The installation needs to be globally such that the 2nd option can be used. But that's already in the installation part of polkadot-launch. So I simply put a Link there. Is that alright?

Copy link
Collaborator

@brenzi brenzi May 5, 2022

Choose a reason for hiding this comment

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

whereas "global" means "across all projects of the current user", not system-wide for all users. then it's ok as it does not demand special privileges
~/.yarn/bin/polkadot-launch

Copy link
Contributor Author

@haerdib haerdib May 6, 2022

Choose a reason for hiding this comment

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

Yes, it's per user. At least if yarn has been installed for per-user. It didn't require any sudo command, in my case, neither for yarn nor for polkadot-launch with -g option.

Copy link
Collaborator

@brenzi brenzi left a comment

Choose a reason for hiding this comment

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

I like the simplification and decluttering.

@brenzi brenzi merged commit 72bb011 into master May 6, 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.

Update Readme
3 participants