-
Notifications
You must be signed in to change notification settings - Fork 267
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
Updating Contributing guidelines and copy-pastable code #2642
Conversation
Signed-off-by: h4l0gen <ks3913688@gmail.com>
Pull Request Test Coverage Report for Build 9258634612Details
💛 - Coveralls |
Hi @jku , we only need to add |
requirements/docs.txt
Outdated
@@ -6,3 +6,4 @@ | |||
# install sphinx and its extensions | |||
sphinx | |||
sphinx-rtd-theme | |||
sphinx-copybutton |
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.
This does add a new doc build dependency, which @jku expressed his reservations about: #2637 (comment)
I don't have strong objections, but I also don't think the copy button is such a big win.
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.
Yeah no strong feelings either. It's true it's not a runtime dependency and it's just one small thing that gets installed when you run tox
. The list of things we install is long already so this doesn't change the situation much... but I don't think we should add developer dependencies without good reason -- and the copy button doesn't feel like a very important one.
The other changes seem good. I would say remove the copybutton references and we'll merge it
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.
That's okay @lukpueh @jku I understands your POV. BTW I've got this idea just to fulfill last point criteria in this heading of CNCF assessment, so that we don't miss this criteria in future. But if you like, I will remove that, and merge other changes, WDYT?
Thank you 👍
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.
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
thanks, squashed and merged. |
…mework#2642) * Make commands easier to copy Signed-off-by: h4l0gen <ks3913688@gmail.com> Signed-off-by: Kapil Sharma <ks3913688@gmail.com> Signed-off-by: shubhusion <shubham27.sharma03@gmail.com>
Description of the changes being introduced by the pull request:
This will make easier to copy commands
Fixes #2637