Skip to content

docs: update README #311

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

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Conversation

RafaelAPB
Copy link
Contributor

The current README presents potentially outdated information that holds details that are better suitable for the whitepaper. It lacks instructions on how to build and run the project, which can frustrate newcomers.

I would propose us to simplify the README of the project, and as the example section matures, provide a practical example.

@RafaelAPB
Copy link
Contributor Author

We can add an updated version of the architecture, and forward readers to the whitepaper, as well

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

LGTM

@petermetz
Copy link
Contributor

@RafaelAPB Would be nice to have a more specific commit message. Update README can mean a lot of different things. I'd say something like docs(readme): reference build instructions for example.

@RafaelAPB
Copy link
Contributor Author

I've chosen that specific message because the commit also removes parts of it. Would it still be reasonable to only refer the build instructions?

@petermetz
Copy link
Contributor

I've chosen that specific message because the commit also removes parts of it. Would it still be reasonable to only refer the build instructions?

Tough question. It definitely makes sense IMO to not just say updated readme, but how much explanation should fit in the commit subject line (the top line) is difficult to say.
My personal take: I'd use the commit body (e.g. lines after the first) to explain the rest of the story like that the theoretic explanations have been removed in favor of the rest of the documentation which contains the same information.
Something like that. :-)

@RafaelAPB
Copy link
Contributor Author

Makes sense. Let's see first what the other reviewers think.

@takeutak takeutak self-requested a review October 16, 2020 10:39
@takeutak takeutak dismissed their stale review October 16, 2020 10:41

Sorry, I mistook to choose review comment.

Copy link
Contributor

@takeutak takeutak left a comment

Choose a reason for hiding this comment

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

I have strengthened your proposal as the commit below. The reason is written in the header of the commit below. Do you have any comments?
3bfab32

@RafaelAPB
Copy link
Contributor Author

I agree, @takeutak ! Thanks!

@petermetz
Copy link
Contributor

CI is failing because of an unrelated problem: #319

Copy link
Contributor

@takeutak takeutak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jonathan-m-hamilton jonathan-m-hamilton left a comment

Choose a reason for hiding this comment

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

LGTM, pending CI resolution

@RafaelAPB
Copy link
Contributor Author

Done @petermetz , thanks!

@takeutak
Copy link
Contributor

takeutak commented Oct 21, 2020

Excuse me, it look like that the current README ignores my suggestion (due to the mistake on solving CI falling?)
3bfab32
Please wait for merging before solving the issue.

@takeutak takeutak self-requested a review October 21, 2020 01:50
Copy link
Contributor

@takeutak takeutak left a comment

Choose a reason for hiding this comment

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

As I said the above, the current status ignores my above commit. Then I canceled my LGTM at once.

@takeutak
Copy link
Contributor

I pushed the following commit, which content is the same as the above my commit.
ab2542d

Now I approved this PR again.

Copy link
Contributor

@takeutak takeutak left a comment

Choose a reason for hiding this comment

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

LGTM

@takeutak takeutak merged commit f0c3cf0 into hyperledger-cacti:master Oct 21, 2020
@RafaelAPB RafaelAPB deleted the documentation branch February 11, 2021 14:06
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.

4 participants