Skip to content
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

jq: update page #14841

Merged
merged 22 commits into from
Dec 2, 2024
Merged

jq: update page #14841

merged 22 commits into from
Dec 2, 2024

Conversation

vanvuvuong
Copy link
Contributor

  • The page(s) are in the correct platform directories: common, linux, osx, windows, sunos, android, etc.
  • The page(s) have at most 8 examples.
  • The page description(s) have links to documentation or a homepage.
  • The page(s) follow the content guidelines.
  • The page(s) follow the style guide.
  • The PR title conforms to the recommended templates.
  • Version of the command being documented (if known):

@github-actions github-actions bot added the page edit Changes to an existing page(s). label Nov 15, 2024
@tldr-bot
Copy link

The build for this PR failed with the following error(s):

pages/common/jq.md:
Error: Parse error on line 5:
...b.io/jq/manual/>.> See also: `trdsql`
----------------------^
Expecting 'INFORMATION_LINK', got 'DESCRIPTION_LINE'
pages/common/jq.md:5: TLDR004 Command descriptions should end in a period

Please fix the error(s) and push again.

@spageektti spageektti changed the title jq: edit page jq: update page Nov 16, 2024
pages/common/jq.md Outdated Show resolved Hide resolved
@vanvuvuong vanvuvuong requested a review from jxu November 20, 2024 04:14
@jxu
Copy link
Collaborator

jxu commented Nov 21, 2024

Did you address my review comments?

@vanvuvuong
Copy link
Contributor Author

Did you address my review comments?

I didn't see your comments, could you please tag me again?

@sebastiaanspeck
Copy link
Member

@jxu pinging you since this PR is waiting for your review. If there will be no response within a week, we will merge this.

@jxu
Copy link
Collaborator

jxu commented Nov 30, 2024

@jxu pinging you since this PR is waiting for your review. If there will be no response within a week, we will merge this.

I already gave a review. It is up to the author to address my comments. I don't see a way to link them so the only thing I can say is scroll up on the issue tracker.

The only comment I have really is to define what "multi-conditional" means. I guess it means combining conditions with AND or OR.

Copy link
Collaborator

@jxu jxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resubmit review

> More information: <https://jqlang.github.io/jq/manual/>.

- Execute a specific expression (print a colored and formatted JSON output):

`{{cat path/to/file.json}} | jq '.'`

- Execute a specific expression only using the `jq` binary (print a colored and formatted JSON output):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we shouldn't have a redundant example just to show the file input, although it is useful. I'll see what the other reviewers say. Related #13668

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one or the first example needs to delete, it is basically the same, just different way of stdin controller.

Copy link
Member

@sebastiaanspeck sebastiaanspeck Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep the second example. The cat-page should show us that this is possible. Not in a non-related page. If someone never views jq, they will never know. It is more likely they will view cat, and then see the | example.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly either way. @vanvuvuong maybe keep the second example and we can get it merged and fix it later if it needs to be

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

pages/common/jq.md Outdated Show resolved Hide resolved
@vanvuvuong vanvuvuong requested a review from jxu November 30, 2024 07:05
@vanvuvuong
Copy link
Contributor Author

I just updated all based on new comments.

@sebastiaanspeck sebastiaanspeck merged commit 87a6144 into tldr-pages:main Dec 2, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
page edit Changes to an existing page(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants