-
Notifications
You must be signed in to change notification settings - Fork 685
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
Update Add Product Guide #10533
Update Add Product Guide #10533
Conversation
Skipping CI for Draft Pull Request. |
b5285bf
to
400f659
Compare
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.
Hello @Mab879 and thank you for this update, it is really helpful because I can imagine it can cause pain for those who try to add new products.
I think that some changes are not needed. Please see my comments.
@@ -411,8 +411,8 @@ MAKEFILE_ID_TO_PRODUCT_MAP = { | |||
|
|||
|
|||
5. Create a new file in the product directory called `CMakeLists.txt`: | |||
``` |
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.
Not sure why this line is changed... I think it should stay the same.
@@ -411,8 +411,8 @@ MAKEFILE_ID_TO_PRODUCT_MAP = { | |||
|
|||
|
|||
5. Create a new file in the product directory called `CMakeLists.txt`: | |||
``` | |||
cat << EOF >> $NEW_PRODUCT/CMakeLists.txt |
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 multiple instances of such changes in this PR. But I think they are not correct - this command is using Bash heredoc to insert contents into a file and the change you propose breaks it.
Heredoc is denoted with <<.
https://www.howtogeek.com/719058/how-to-use-here-documents-in-bash-on-linux/
@@ -437,7 +437,7 @@ pkg_manager: "yum" | |||
|
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.
@Mab879 I think additional .. needs to be here.
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 update you requested should be in now.
Code Climate has analyzed commit 42d7ed4 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 52.5% (0.1% change). View more on Code Climate. |
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.
Looks good now. Thank you.
Description:
Update add products guide.
Rationale:
Make life easier for developers.