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

find: fix two examples with wrong or misleading syntax #15108

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zmajeed
Copy link

@zmajeed zmajeed commented Dec 9, 2024

  • 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):

Fixes #15107

@CLAassistant
Copy link

CLAassistant commented Dec 9, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the page edit Changes to an existing page(s). label Dec 9, 2024
@@ -33,4 +33,4 @@

- Find empty files (0 byte) or directories and delete them verbosely:

`find {{root_path}} -type {{f|d}} -empty -delete -print`
`find {{root_path}} -type '{{f,d}}' -empty -delete -print`
Copy link
Member

Choose a reason for hiding this comment

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

This example is not wrong :D I'd rather leave it like this. The | is fine, because you either have a f (for file) or a d for directory there.

Copy link
Author

Choose a reason for hiding this comment

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

Have you tried to run the example?

Copy link
Member

Choose a reason for hiding this comment

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

The | is tldr syntax (similar to {{}}), so users would change the command accordingly to either contain -type f or -type d.

However, I realise that the current form of the command is indeed misleading. I would wait what others have to say on this issue.

Copy link
Author

Choose a reason for hiding this comment

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

No problem - though I think the example text Find empty files (0 byte) or directories and delete them verbosely is pretty specific and doesn't make sense without the corrected syntax

Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the event that made you feel that this command needed to be changed? I want to understand if it was a clarity issue or if it's just misunderstanding for not knowing the syntax.

Copy link
Author

Choose a reason for hiding this comment

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

The command for the example is wrong - the syntax is wrong in two ways - I've fixed it to run and correctly work for the described purpose

Copy link
Collaborator

Choose a reason for hiding this comment

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

image
Works fine for me.

Copy link
Author

Choose a reason for hiding this comment

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

it skips directories

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well yes, the description does say "or". It can be interpretted in two ways I guess. I'll ruminate on this for a while and try to come up with a a better wording. I do think f|d is the way to go, just that the wording needs to be more precise that the user needs to choose.

@@ -9,7 +9,7 @@

- Find files matching multiple path/name patterns:

`find {{root_path}} -path '{{**/path/**/*.ext}}' -or -name '{{*pattern*}}'`
`find {{root_path}} -path '{{*/path/*/*.ext}}' -or -name '{{*pattern*}}'`
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, thanks!

@@ -33,4 +33,4 @@

- Find empty files (0 byte) or directories and delete them verbosely:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Find empty files (0 byte) or directories and delete them verbosely:
- Find empty files or directories and delete them verbosely:

I feel like the parenthese creates an unnecessary gap before the word "or"

Copy link
Author

Choose a reason for hiding this comment

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

Agree - "(0 byte)" is clunky and ugly

Copy link
Author

Choose a reason for hiding this comment

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

I like this better "List and delete empty files and directories"

@@ -33,4 +33,4 @@

- Find empty files (0 byte) or directories and delete them verbosely:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Find empty files (0 byte) or directories and delete them verbosely:
- Search for either empty files or directories and delete them verbosely:

This maybe?

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.

Page modification request: find
4 participants