-
-
Notifications
You must be signed in to change notification settings - Fork 37.8k
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
Better filter and documentation for qmk find #23711
base: master
Are you sure you want to change the base?
Conversation
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.
Logic looks good to me, some feedback on code style.
@@ -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. |
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.
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__())
@@ -61,10 +61,22 @@ def apply(self, target_info: TargetInfo) -> bool: | |||
|
|||
class Length(FilterFunction): | |||
func_name = "length" | |||
funcMap = [ |
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.
Just a nitpick, camelCase looks off when everything else is snake_case... Its the "official" format for vars anyway.
|
||
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())) |
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.
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
or3.2
will be parsed as32
- 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
Thank you for your contribution! |
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist