-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
crane/gcrane: add pages #13853
base: main
Are you sure you want to change the base?
crane/gcrane: add pages #13853
Conversation
#13808 also adds |
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.
First of all, thank you for the amazing work you put into this! Always great to have a first-contributor on board.
I have reviewed upon crane-index-filter.md
, but those comments also apply to the rest of the files.
- Generally we put Display help and Display version as last commands (https://github.com/tldr-pages/tldr/blob/main/contributing-guides/style-guide.md#help-and-version-commands).
- A single option, e.g.
--help
, does not to be enclosed in{{...}}
, only placeholders and short/long options{{-h|--help
}}`. - I also see a lot of
{{strings}}
, please be more descriptive what the string represents, a tag name, a filepath etc. - If you can have more than 1 argument, please use
{{argument1, argument2 ...}}
.
Hi thanks for the extensive review. I'll implement your feedback after the weekend and then we can hopefully merge soon after! Cheers |
03bf275
to
e4dde03
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Hi I've updated the PR according to the guidelines and your feedback can you take another look? Furthermore, as the bot indicates I've added the various implementations of |
Hello! I've noticed something unusual when checking this PR:
Is this intended? If so, just ignore this comment. Otherwise, please double-check the commits. |
Add some suggested improvements Co-authored-by: Sebastiaan Speck <[email protected]>
… gcrane completion cmd
Co-authored-by: Sebastiaan Speck <[email protected]>
7246b78
to
dd2d508
Compare
Hello! I've noticed something unusual when checking this PR:
Is this intended? If so, just ignore this comment. Otherwise, please double-check the commits. |
1 similar comment
Hello! I've noticed something unusual when checking this PR:
Is this intended? If so, just ignore this comment. Otherwise, please double-check the commits. |
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.
Besides the extra comments, reviewing is easier if you do not force push the branch. That way we can see the changes per commit, especially if it is a large PR like this.
As we have OS specific implementation
Co-authored-by: Sebastiaan Speck <[email protected]>
Ah yes sorry, I though rebasing the branch with main was the way to go, but I get that this complicates the review. I've updated your last comments. |
closes #13664
common
,linux
,osx
,windows
,sunos
,android
, etc.