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

Upgrade readme #22

Merged
merged 24 commits into from
Apr 25, 2023
Merged

Upgrade readme #22

merged 24 commits into from
Apr 25, 2023

Conversation

jcbhmr
Copy link
Collaborator

@jcbhmr jcbhmr commented Mar 11, 2023

This PR would... (checkboxes are draft progress)

Why this is a good idea:

  1. It has header image. Images are worth 1000 words
  2. It has splashes of color. colors are good for human eyeballs to draw attention
  3. It has more than one code example. This helps readers generalize instead of pigeonhole on a single usecase
  4. It has a well-formatted table. This is personal preference of formatting.

@jcbhmr jcbhmr mentioned this pull request Mar 11, 2023
jcbhmr added 2 commits March 11, 2023 22:10
Used official phrase from the https://github.com/marketplace?type=actions for any action "$NAME is not certified by GitHub. It is provided by a third-party and is governed by separate terms of service, privacy policy, and support documentation."
@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 11, 2023

Options for the inputs:

1: table

image

2: In YAML

image
https://github.com/actions/checkout#readme

3: Literal list

image
https://github.com/wow-actions/contributors-list#readme

4: Headers for each option

image
https://github.com/veracode/veracode-sandboxes-helper#readme

5: dl description list

<dl> (couldn't find any GitHub Action examples of this)
image

@Andrew-Chen-Wang opinions?

@jcbhmr jcbhmr marked this pull request as ready for review March 11, 2023 22:21
@jcbhmr jcbhmr added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 11, 2023
@jcbhmr jcbhmr linked an issue Mar 11, 2023 that may be closed by this pull request
@jcbhmr jcbhmr modified the milestones: Test if node will be faster, v4 Mar 13, 2023
@Andrew-Chen-Wang
Copy link
Owner

veracode lgtm with individual sections for paragraphs

@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 13, 2023

veracode lgtm with individual sections for paragraphs

Sounds good! 👍 Individual sections for each option it is.

@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 13, 2023

Here's a preview screnshot of what it looks like:

image

It's OK in my opinion... I think that it takes up a bit much vertical space, but that's my opinion.

Good enough for now though!

@Andrew-Chen-Wang
Copy link
Owner

My thoughts are: if we have a huge repository that have multiple functionalities, then it'll require quite a bit of explaining.

@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 13, 2023

btw how do I merge this? Do you have to merge it? Why does it say 1 review required? I'm a bit of a newbie

image

I'm assuming your "lgtm" comment is an endorsement that this PR is OK to merge... If I'm misunderstanding please correct me ❤

@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 13, 2023

Just spotted that I used my old header image from jcbhmr/deploy-wiki and didn't update the text in photosohp... I need to fix that 😆

@Andrew-Chen-Wang
Copy link
Owner

yea I set it to have at least one review required

Copy link
Owner

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

a few comments for now. A lot on my plate at the moment

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 13, 2023

Oh and when if this is merged set the description of the repo to the same as the desc in the readme to satisfy my ocd pretty pls.

📖 GitHub Action to sync a folder to the GitHub wiki

@jcbhmr jcbhmr requested a review from Andrew-Chen-Wang March 21, 2023 02:06
Copy link
Owner

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Thanks a bunch! Looks great!

README.md Outdated
Comment on lines 10 to 12
**GitHub Wiki Action** is not certified by GitHub. It is provided by a
third-party and is governed by separate terms of service, privacy policy, and
support documentation.
Copy link
Owner

Choose a reason for hiding this comment

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

We don't have a terms of service. You can just keep the original notice that the repository is not affiliated with GitHub

Copy link
Collaborator Author

@jcbhmr jcbhmr Mar 23, 2023

Choose a reason for hiding this comment

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

really i don't even know why its needed since it appears on the page sidebar as a disclaimer added by github. i just copied that into this repo since I figured you wanted it (it was already kinda there at the bottom) 🤷

image

https://github.com/marketplace/actions/github-wiki-action

Copy link
Collaborator Author

@jcbhmr jcbhmr Mar 23, 2023

Choose a reason for hiding this comment

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

i don't think this is required because literally no other action does it. it's only there on github's marketplace to indicate that the github.com skin thing with github branding and stuff doesn't mean they endorse your product.

you don't need that notice in your readme though since that's already covered under other terms/conds as being yours not githubs (i think?) -- it only needs to appear added as a sidebar by the github.com/marketplace site when the readme is being presented on their own branded page, but not when its just your repo page

at least that's my understanding of it?


Unless required by applicable law or agreed to in writing, software
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep license notice at the bottom

Copy link
Collaborator Author

@jcbhmr jcbhmr Mar 23, 2023

Choose a reason for hiding this comment

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

isnt that in the LICENSE file tho? 🤔

and shouldn't the license thing:

Copyright [yyyy] [name of copyright owner]

   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
   You may obtain a copy of the License at

       http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.

go in the actual code files not the readme?

Copy link
Collaborator Author

@jcbhmr jcbhmr Mar 23, 2023

Choose a reason for hiding this comment

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

actually i found an authoritative source https://infra.apache.org/apply-license.html that states

Each original source document (code and documentation, but not the LICENSE and NOTICE files) should include a short license header at the top. If the distribution contains documents not covered by an ICLA, CCLA or Software Grant (such as third-party libraries), consult the policy guide.

or https://www.apache.org/foundation/license-faq.html#Apply-My-Software

image

Copy link
Owner

@Andrew-Chen-Wang Andrew-Chen-Wang Mar 24, 2023

Choose a reason for hiding this comment

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

Typically, all that's necessary is the LICENSE file. Putting the header in the source files are also a plus but not needed. I always have license information in the README for quick access purpose when reading through READMEs. It's fairly standard in the OSS world, perhaps not the boilerplate that I like including, but mentioning the license is always helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Putting the header in the source files are also a plus but not needed.

it actually sounds like its required in the apache 2.0 license

@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Mar 23, 2023

I've added the following license header to each code or documentation item in the repo:

Copyright 2023 Andrew Chen Wang
SPDX-License-Identifier: Apache-2.0

as stated here:

image

and as discussed here: open-telemetry/community#113 (comment)

@jcbhmr jcbhmr requested a review from Andrew-Chen-Wang March 24, 2023 19:59
@jcbhmr
Copy link
Collaborator Author

jcbhmr commented Apr 24, 2023

@Andrew-Chen-Wang 📬

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit e8a2499 into Andrew-Chen-Wang:master Apr 25, 2023
@jcbhmr jcbhmr deleted the upgrade-readme branch April 25, 2023 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade readme
2 participants