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: remove the lists of packages from the readme #969

Merged
merged 6 commits into from
Apr 18, 2022

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Apr 11, 2022

Which problem is this PR solving?

Every now and then we get a PR for updating the list in the Readme. I don't personally feel having a list in the readme is worth it if we can reference the folder the packages are all nested in but I'm also interested in what everyone else thinks.

Short description of the changes

  1. Remove the lists of the included packages from the readme.
  2. Reword some other parts about the support according to Discussion: Drop support for older Node.JS versions opentelemetry-js#2721 (comment)

@rauno56 rauno56 requested a review from a team as a code owner April 11, 2022 14:06
@vmarchaud
Copy link
Member

Apart from the lint failing, i agree that its a problem and the information is generally available elsewhere (specially in the opentelemtry website) so i'm in favor of removing them as-is and maybe replace them by a link to https://opentelemetry.io/registry/ ?

@dyladan
Copy link
Member

dyladan commented Apr 11, 2022

I agree a link to the registry should be sufficient. I wonder if it would be possible to have a CI check that warns if the package is not in the registry?

@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #969 (0f28d5f) into main (b55b79b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #969   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files          13       13           
  Lines         856      856           
  Branches      178      178           
=======================================
  Hits          821      821           
  Misses         35       35           

@rauno56
Copy link
Member Author

rauno56 commented Apr 12, 2022

Added a link to the registry.

I think linking straight to Registry in the main content of the readme creates weird UX(repo -> registry -> repo) with little benefit(registry search vs Ctrl-F).

I have a feeling that most people will arrive to the repo through npm anyway

@dyladan
Copy link
Member

dyladan commented Apr 12, 2022

Added a link to the registry.

I think linking straight to Registry in the main content of the readme creates weird UX(repo -> registry -> repo) with little benefit(registry search vs Ctrl-F).

I have a feeling that most people will arrive to the repo through npm anyway

I think its fine how you did it. The registry links directly to packages though. Unless someone is already familiar with our package structure, they may find the registry easier to navigate. In any case this LGTM

@rauno56 rauno56 merged commit 7800561 into open-telemetry:main Apr 18, 2022
@rauno56 rauno56 deleted the docs/readme-remove-lists branch April 18, 2022 13:27
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.

None yet

4 participants