-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add new command to list flags for documentation #165
base: main
Are you sure you want to change the base?
Conversation
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.
Hi @omerap12, sorry for the long delay.
This looks great, thank you!
/lgtm
/cc @LiorLieberman @mlavacca
sure, no problem :) |
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.
Hey, I understand your point. My intention was to add this flag to enable automated generation of the README.md (saving the output to a file when the flag is called). I wanted to break this into smaller steps rather than submitting a large PR all at once. I can continue with this approach if you allow me to. :) |
I saw no mention about this approach neither in the PR description nor in the code, so I assumed this was the contribution to fix #162. Splitting this task into sub-tasks sounds good to me, but I ask you to clarify such in the PR description and to change "fixes #issue" to something like "part of #issue", otherwise the issue will be closed by the merge of this PR. |
Yes, you are right sorry about that. fixing it now |
No worries, and thanks again for this contribution! |
It’s my pleasure! :) I’ll adjust the code according to your review later. |
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Omer Aplatony <[email protected]>
should I set this? table.SetRowLine(true) |
/cc @mlavacca for review |
@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: for, review. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/assign |
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.
Hi @omerap12, I somehow missed this PR, sorry about that.
I know you implemented this after my suggestion, but after a thorough look, using this table does not make much sense to me: we want it to be used in a .md
file, therefore it should be proper .md
code, and not this custom terminal-oriented format.
I think your first implementation was in the right direction, we just need to find a way to avoid the hardcoded formatting. Apologise for all the back and forth on this.
Sure, no problem. Could you please provide an example of how you'd like the output to look? |
Your first implementation was in the right direction, we just need to find a good way not to have hardcoded table formatting, as it can lead to many problems. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
What type of PR is this?
/kind documentation
/kind feature
-->
What this PR does / why we need it:
Adds list-flags command to print flag details directly.
usage:
Output:
Which issue(s) this PR fixes:
As part of issue #162, we're adding a new command to list flags for documentation. This will aid in automating the process of generating a list of available flags for creating a README.md file.
Does this PR introduce a user-facing change?: