-
Notifications
You must be signed in to change notification settings - Fork 383
fix: make sure generated readmes for lb4 have correct links #608
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
Conversation
| # | ||
| (cat <<LIST_END | ||
| strongloop loopback-next master metadata README.md | ||
| strongloop loopback-next master packages/metadata/README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be altered to include all of the packages? It seems like we're only pointing at metadata with this setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For LB4, we only pull in README for metadata at this moment. This is the place you can add more readmes to be imported.
bschrammIBM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are probably aware of this, but the Commit Linter failed.
c82cac7 to
a35a8a1
Compare
This is a follow-up to #588
a35a8a1 to
5bb4d1a
Compare
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it's a good idea to support both "npm module" and "github branch" modes. What are the rules/guidelines for deciding what source to use when? Could you please describe them in the comment inside update-v4-readmes.sh?
Also, have you verified that "npm module" downloads actually work - that they put the contents to a right place and that the generated link is pointing to the right npm package?
| # branch name will be appended to the local readme file name. | ||
| # | ||
| # Examples: | ||
| # strongloop loopback-next master packages/metadata/README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will not work because we are not publishing loopback-next to npmjs.org. Unless I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only pull from npmjs if the module field (last one) is specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Could you please enhance the comments to explain what is each example showing?
Maybe we should use the following rule?
Thoughts? |
I didn't invent that. The PR follows what we do for LB v3.
Yes. I did. See the following comments in the script: |
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes looks reasonable, I don't see any obvious problem.
I think we should rethink the way how we are mirroring READMEs, but I guess that's beyond the scope of this pull request.
This is a follow-up to #588.
The PR introduces
page.fileto customize the location of the README file.The
update-v4-readmes.shscript now downloads README files intopages/en/lb4/readmes/loopback-next/packages/<pkg>/README.mdand thereadme.htmlJekyll template honorspage.filesetting.For example:
The template will use
loopback-next/packages/metadata/README.mdas the file location andhttps://github.com/strongloop/loopback-next/blob/master/packages/metadata/README.mdas the link.