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.

Sign up for GitHub

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

Jump to bottom

Better filter and documentation for qmk find #23711

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Better filter and documentation for qmk find #23711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Better filter and documentation for qmk find
  • Loading branch information
@jvbroek
jvbroek committed May 13, 2024
commit f7d902e6198586a4d53326cdaed2410076c1a8fc
6 changes: 3 additions & 3 deletions docs/cli_commands.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,18 @@ For example, one could search for all keyboards using STM32F411:
qmk find -f 'processor=STM32F411'
```

...and one can further constrain the list to keyboards using STM32F411 as well as rgb_matrix support:
...and one can further constrain the list to keyboards using STM32F411 as well as rgb_matrix support with at least 3 columns:

```
qmk find -f 'processor=STM32F411' -f 'features.rgb_matrix=true'
qmk find -f 'processor=STM32F411' -f 'features.rgb_matrix=true' -f 'length(matrix_pins.cols, >=3)'
```

The following filter expressions are also supported:

- `exists(key)`: Match targets where `key` is present.
- `absent(key)`: Match targets where `key` is not present.
- `contains(key, value)`: Match targets where `key` contains `value`. Can be used for strings, arrays and object keys.
- `length(key, value)`: Match targets where the length of `key` is `value`. Can be used for strings, arrays and objects.
- `length(key, value)`: Match targets where the length of `key` matches `value`. Can be used for strings, arrays and objects.

You can also list arbitrary values for each matched target with `--print`:

Expand Down
2 changes: 1 addition & 1 deletion lib/python/qmk/cli/find.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Command to search through all keyboards and keymaps for a given search criteria.

Check failure on line 1 in lib/python/qmk/cli/find.py

View workflow job for this annotation

GitHub Actions / lint 

Requires Formatting
"""
from milc import cli
from qmk.search import filter_help, search_keymap_targets
Expand All @@ -11,7 +11,7 @@
action='append',
default=[],
help= # noqa: `format-python` and `pytest` don't agree here.
f"Filter the list of keyboards based on their info.json data. Accepts the formats key=value, function(key), or function(key,value), eg. 'features.rgblight=true'. Valid functions are {filter_help()}. May be passed multiple times; all filters need to match. Value may include wildcards such as '*' and '?'." # noqa: `format-python` and `pytest` don't agree here.
f"Filter the list of keyboards based on their info.json data. Accepts the formats key=value, function(key), or function(key,value), eg. 'features.rgblight=true' or 'length(encoder.rotary, >=20)'. Valid functions are {filter_help()}. May be passed multiple times; all filters need to match. Value may include wildcards such as '*' and '?'." # noqa: `format-python` and `pytest` don't agree here.
Copy link
Contributor

@elpekenin elpekenin May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I dont like the special handling here. Imo everything should come from filter_help and be self-contained on the classes

Could implement a get_help() function on the base class, as simply returning the function's name by default, and overwrite when needed. Then, for the sake of readability, print them as (instead of the current single-line approach)

* One function
* Another function
    Extra docs

Where the indentation is handled by the class adding a long description and the "common" code looks like
"\n * ".join(klass.get_help() for klass in FilterFunction.__subclasses__())

)
@cli.argument('-p', '--print', arg_only=True, action='append', default=[], help="For each matched target, print the value of the supplied info.json key. May be passed multiple times.")
@cli.argument('-km', '--keymap', type=str, default='default', help="The keymap name to build. Default is 'default'.")
Expand Down
14 changes: 13 additions & 1 deletion lib/python/qmk/search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""Functions for searching through QMK keyboards and keymaps.

Check failure on line 1 in lib/python/qmk/search.py

View workflow job for this annotation

GitHub Actions / lint 

Requires Formatting
"""
import contextlib
import functools
Expand Down Expand Up @@ -61,10 +61,22 @@

class Length(FilterFunction):
func_name = "length"
funcMap = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nitpick, camelCase looks off when everything else is snake_case... Its the "official" format for vars anyway.

(">=", int.__ge__),
("<=", int.__le__),
(">", int.__gt__),
("<", int.__lt__),
]

def apply(self, target_info: TargetInfo) -> bool:
_kb, _km, info = target_info
return (self.key in info and len(info[self.key]) == int(self.value))
val = int(''.join(c for c in self.value if c.isdigit()))
Copy link
Contributor

@elpekenin elpekenin May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As far as i can tell this is intended to convert the (potentially) {<,<=,>,>=}number into an actual int, but i dont like it because of two things:

  • You will mistakenly (and silently) convert weird inputs into numbers, eg: 3a2 or 3.2 will be parsed as 32
  • Is (IMO) much less readable than just doing
for start, func in self.funcMap: # i would also name this something like "operator"
    if self.value.startswith(start):
        comp = func
        val = int(self.value[len(start):]  # remove prefix, then convert to int (or fail trying to do so)

Also, the extra parenthesis on the for, if and return look a bit off, but just code style yet again

comp = int.__eq__
for (start, func) in self.funcMap:
if (self.value.startswith(start)):
comp = func
break
return (self.key in info and comp(len(info[self.key]), val))


class Contains(FilterFunction):
Expand Down
Loading