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

Add --filter to many commands #3718

Merged
merged 18 commits into from Oct 16, 2018
Merged

Add --filter to many commands #3718

merged 18 commits into from Oct 16, 2018

Conversation

greg-1-anderson
Copy link
Member

@greg-1-anderson greg-1-anderson commented Oct 4, 2018

$ drush9 pml --filter='id=bartik' 
 --------- ----------------- --------- --------- 
  Package   Name              Status    Version  
 --------- ----------------- --------- --------- 
  Core      Bartik (bartik)   Enabled   8.5.3    
 --------- ----------------- --------- --------- 

Just one field:

$ drush9 pml --filter='id=bartik' --field=status
Enabled

@greg-1-anderson
Copy link
Member Author

It might be possible to make the @filter-output implicit based on the return type of the command; however, if we did this, then there should be some other annotation to turn off the filter. If a command is already filtering the output rows, then it would be inefficient to pass it through the generic filter as well.

I don't think it would be too bad to have to add the annotation to commands that need it.

@greg-1-anderson
Copy link
Member Author

I updated the documentation for filter-via-dot-access-data to illustrate how "contains" and "regex" searches are done.

@greg-1-anderson
Copy link
Member Author

The role:list command already took a --filter parameter that had different conventions than the filter added by this PR. To compensate, I added a feature to consolidation/filter-via-dot-access-data that allows role:list to specify that the default field to filter by is "perms". If someone uses --filter=value without providing a filter operator, then "perms contains value" is the operation that is performed. This allows both:

  • drush role:list --filter='edit terms in tags'
  • drush role:list --filter='perms*=edit terms in tags'
  • drush role:list --filter='id=editor'

@@ -0,0 +1,36 @@
Output Filters
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for writing this ... I have two thoughts. I don't know how to handle them though.

  1. Output filters sounds a lot like output formats which is a related but different concept. Everyone is going to mix them up.
  2. Might be worth saying how this feature differs from grep and when to use each.

src/Commands/core/UpdateDBCommands.php Outdated Show resolved Hide resolved
@greg-1-anderson
Copy link
Member Author

Output filters sounds a lot like output formats which is a related but different concept. Everyone is going to mix them up.

I resolved this by making one topic on adjusting command output. It covers output formats, output fields and output filters together. Since all of these options are related to the same concept, it should be clear if folks can read about them together.

Might be worth saying how this feature differs from grep and when to use each.

Haven't worked this in yet.

  • Output filters are like grep, but they match only the data within specific named fields. This makes it easier to avoid false matches.
  • Filters are also applied before data is formatted, so you don't need to worry about missing out on rows that should have matched, but that were missed because the matching data was word-wrapped.
  • Use grep directly if you need to match based on data that spans columns.

Something akin to that could go in the introduction, or maybe at the top of the "output filters" section.

Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

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

I think the option help text --filter[=FILTER] Filter output based on provided expression would benefit from an example. It could be a generic example --filter=<fieldname>=foo or customized for that command <display_name*=Ctools>.

That brings up a question do the = and *= operators perform case insensitive comparisons? I think I would prefer that.

Also, I think the value is required for all these options, not just --field:

--format[=FORMAT]
--field=FIELD
--filter[=FILTER]

@@ -31,6 +31,7 @@
* @command {{ machine_name }}:token
* @aliases token
*
* @filter-output
Copy link
Member

Choose a reason for hiding this comment

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

should declare a default field instead

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we pick which field is the default field? Assume id and let the user change it?

Copy link
Member

Choose a reason for hiding this comment

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

In this case I would go with “name”. This is just an example method. It’s not dynamically adjusted or anything.

glossary
```

Output Filters
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that this topic should be offerred automatically in help for any command that shows the --fields option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@greg-1-anderson
Copy link
Member Author

The = and *= are always case-insensitive. ~= is case-insensitive if you tack an i on at the end of the regex, e.g. foo~=#expr#i

Regarding --filter[=FILTER], it seems that the annotated command does not offer a good way to stipulate options with required values via annotations. We must fix add that to the library before we can enhance here.

@greg-1-anderson
Copy link
Member Author

I think I've done about as much as I intend to do here for now. I recommend that we first release Drush 9.5.0, and then merge this PR to the master branch so that folks can try it out in the dev release for a while before calling it stable.

@@ -31,6 +31,7 @@
* @command {{ machine_name }}:token
* @aliases token
*
* @filter-output
Copy link
Member

Choose a reason for hiding this comment

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

In this case I would go with “name”. This is just an example method. It’s not dynamically adjusted or anything.

@@ -112,6 +112,7 @@ public function entityUpdates($options = ['cache-clear' => true])
* description: Description
* type: Type
* @default-fields module,update_id,type,description
* @filter-default-field module
Copy link
Member

Choose a reason for hiding this comment

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

I think 'type' would be a better default field here.

@weitzman weitzman changed the title Add --filter to pm:list Add --filter to many commands Oct 15, 2018
@greg-1-anderson
Copy link
Member Author

Before I merge here, I need to add an example to the --fields help for unstructured data, and I also need to make a stable release of the output-formatters project. The later is ready save only for the addition of another example or two in the documentation for the new features.

…. Fine-tune a couple default fields for the filter option.
@greg-1-anderson
Copy link
Member Author

This should be ready to go after tests pass.

@greg-1-anderson greg-1-anderson merged commit 3e4d5ae into master Oct 16, 2018
@weitzman weitzman deleted the output-filter branch December 13, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants