Skip to content
This repository has been archived by the owner on Dec 20, 2022. It is now read-only.

Re-orient top-level documentation toward new developers #97

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

kfogel
Copy link
Contributor

@kfogel kfogel commented Sep 14, 2021

Purpose

Make the project friendlier to newcomers.

Change summary

Reorganize and lightly rewrite the top-level README.md and CONTRIBUTING.md so that they are aimed more at a newcomer audience -- developers who have heard of DSNP but doesn't know much about it yet. The README now has a table of contents and starts out by stating the purpose of the Example Client and who the intended audience is.

@kfogel kfogel added the documentation Improvements or additions to documentation label Sep 14, 2021
@kfogel
Copy link
Contributor Author

kfogel commented Sep 14, 2021

Do we want to link to https://libertydsnp.github.io/example-client/ from the README, by the way?

Copy link

@rabble rabble left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@claireolmstead
Copy link
Collaborator

A lot of great changes!

@shannonwells
Copy link
Collaborator

shannonwells commented Sep 15, 2021

Do we want to link to https://libertydsnp.github.io/example-client/ from the README, by the way?

I think that's a good idea.

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this, I just made a couple of corrections and suggestions.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@kfogel
Copy link
Contributor Author

kfogel commented Sep 21, 2021

Suggestion: Even without the changes suggested above, this PR is an improvement (I think) on the docs there currently. How about we merge to main, and then I do a separate PR to address what I'm able to from the commentary above.

The reason I suggest doing it that way is that most of the comments above are about further improvements; they're not bugs in the PR so much as things we could do in addition to this PR. When that's the case, I think merge-and-improve is the best course, because that way the perfect is not being the enemy of the good and we get the stuff that's already done into the mainline of development.

Thoughts?

@kfogel
Copy link
Contributor Author

kfogel commented Sep 21, 2021

Also, I just said this to @shannonwells in another channel:

"But if you'd like to tweak the doc changes -- which you are far more qualified to do than I am anyway -- +1 and just put the tweaks directly on the branch before merging!"

@shannonwells
Copy link
Collaborator

shannonwells commented Sep 21, 2021

I'm good to approve if we only remove or change the info about static-server, which I'm fine doing myself just so we can get it merged. At this point though it'll have to be rebased first. Also happy to do that too given your state of busy-ness right now.

@kfogel
Copy link
Contributor Author

kfogel commented Sep 21, 2021

Ah, you're right -- that's an actual technical mistake in my change that does make things worse :-), good point. Yes, please go for it.

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Marking approved so we can get this merged and continue in another PR with some of the other suggestions (internal and external)

@kfogel
Copy link
Contributor Author

kfogel commented Sep 24, 2021

Ah, I merged locally but I can't push to main:

$ git branch
* main
  newcomer-doc-updates
$ git merge newcomer-doc-updates
hint: Waiting for your editor to close the file...
Waiting for Emacs...
Merge made by the 'recursive' strategy.
 CONTRIBUTING.md |  97 +++++++++++++++++++++++++++++++++++++++++++++++----------------
 README.md       | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------
 2 files changed, 169 insertions(+), 96 deletions(-)
$ git push origin main:main
Enumerating objects: 20, done.
Counting objects: 100% (16/16), done.
Delta compression using up to 4 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (11/11), 6.10 KiB | 6.10 MiB/s, done.
Total 11 (delta 7), reused 9 (delta 6), pack-reused 0
remote: Resolving deltas: 100% (7/7), completed with 5 local objects.        
remote: error: GH006: Protected branch update failed for refs/heads/main.        
remote: error: 2 of 2 required status checks are expected.        
To github.com:LibertyDSNP/example-client.git
 ! [remote rejected] main -> main (protected branch hook declined)
error: failed to push some refs to 'github.com:LibertyDSNP/example-client.git'
$ 

It merges cleanly, FWIW, so whenever you get a chance it shouldn't take hardly any time, @shannonwells or @claireolmstead.

@sbendar sbendar merged commit 86c7777 into LibertyDSNP:main Sep 27, 2021
claireolmstead pushed a commit that referenced this pull request Sep 27, 2021
* Revamp top-level docs for a newcomer audience

* A few tweaks and a correction to purpose of static-server

Co-authored-by: shannonwells <shannonwells@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants