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

qmk find: Fix handling of functions in filters #21090

Merged
merged 1 commit into from
May 30, 2023

Conversation

sigprof
Copy link
Contributor

@sigprof sigprof commented May 30, 2023

Description

Functions in filters did not work properly except when used in the last (or only) filter. The problem was caused by the peculiarity of the lambda behavior in Python — any variables from the outer scope are captured only by reference, therefore any subsequent reassignment of those variables is propagated to all lambdas created earlier in the same scope. Together with the laziness of filter() (it returns an iterator which performs filtering on demand) this resulted in all function filters using the values of the key and value variables which correspond to the last filter in the sequence, therefore the result of filtering was wrong if some filter with a function was not the last one in the sequence.

Apparently the shortest way to make a Python lambda capture some variables by value is to add arguments with default values for such variables (default values are evaluated when the lambda is created, and any subsequent reassignments in the outer scope no longer changes them). This makes filters with functions work properly even when such filters are not at the last position in the sequence.


An alternate solution is to wrap all filter() calls with list() to force immediate evaluation — in that case the fact that lambda captures variables from the outer scope by reference does not matter, because the lambda is invoked only while those variables still have the expected values. However, in this case the actual problem (the need to capture values instead of references) is not immediately obvious from the code.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

These commands gave different results (the first one correctly printed some keyboard names matching the specified filters, while the second one, which differs only in the order of -f options, did not print anything):

qmk find -f 'split.enabled=true' -f 'features.rgblight=true' -f 'absent(rgblight.split_count)'
qmk find -f 'absent(rgblight.split_count)' -f 'split.enabled=true' -f 'features.rgblight=true'

With the change in this PR both commands give the same result.

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Functions in filters did not work properly except when used in the last
(or only) filter.  The problem was caused by the peculiarity of the
`lambda` behavior in Python — any variables from the outer scope are
captured only by reference, therefore any subsequent reassignment of
those variables is propagated to all lambdas created earlier in the same
scope.  Together with the laziness of `filter()` (it returns an iterator
which performs filtering on demand) this resulted in all function
filters using the values of the `key` and `value` variables which
correspond to the last filter in the sequence, therefore the result of
filtering was wrong if some filter with a function was not the last one
in the sequence.

Apparently the shortest way to make a Python lambda capture some
variables by value is to add arguments with default values for such
variables (default values are evaluated when the lambda is created, and
any subsequent reassignments in the outer scope no longer changes them).
This makes filters with functions work properly even when such filters
are not at the last position in the sequence.
@github-actions github-actions bot added cli qmk cli command python labels May 30, 2023
@elpekenin
Copy link
Contributor

elpekenin commented May 30, 2023

Could be benefitial moving away from if/else into a name: lamba mapping, and perhaps use functools.partial for the "capturing"(maybe there are some optimizations on it?) My proposal would be something like (not tested yet)

FUNCTIONS = {
    'length': lambda e, k, v: k in e[2] and len(e[2].get(k)) == int(v),
    'contains': lambda e, k, v: k in e[2] and v in e[2].get(k),
    'exists': lambda e, k, v: k in e[2],
    'absent': lambda e, k, v: k not in e[2],
}

...

                if function_match is not None:
                    func_name = function_match.group('function').lower()
                    key = function_match.group('key')
                    value = function_match.group('value')

                    filter_fn = FUNCTIONS.get(func_name)
                    if filter_fn is None:
                        cli.log.warning(f'Unrecognized filter expression: {function_match.group(0)}')
                        continue

                    valid_keymaps = filter(partial(filter_fn, k=key, v=value), valid_keymaps)

                    cli.log.info(f'Filtering on condition: {{fg_green}}{func_name}{{fg_reset}}({{fg_cyan}}{key}{{fg_reset}}, {{fg_cyan}}{value}{{fg_reset}})...')

                elif equals_match is not None:

@zvecr
Copy link
Member

zvecr commented May 30, 2023

Merging as is as its fairly isolated small fixes. Any re-writing of functionallity would have to go to develop.

@zvecr zvecr merged commit 1411c79 into qmk:master May 30, 2023
@elpekenin elpekenin mentioned this pull request May 31, 2023
14 tasks
freznel10 added a commit to freznel10/qmk_firmware that referenced this pull request Jun 2, 2023
commit ef788c6
Merge: aa33fb0 ae5bcaa
Author: QMK Bot <hello@qmk.fm>
Date:   Fri Jun 2 07:46:36 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit ae5bcaa
Author: yiancar <yiangosyiangou@cytanet.com.cy>
Date:   Fri Jun 2 08:45:54 2023 +0100

    [keyboard] Phoenix (qmk#21051)

    * Update keyboards/cablecardesigns/phoenix/

commit aa33fb0
Author: Joel Challis <git@zvecr.com>
Date:   Fri Jun 2 02:45:48 2023 +0100

    Revert "Add *_MATRIX_LED_COUNT generation/validation (qmk#19515)" (qmk#21109)

    This reverts commit 25c16b3.

commit 25c16b3
Author: Joel Challis <git@zvecr.com>
Date:   Fri Jun 2 02:42:49 2023 +0100

    Add *_MATRIX_LED_COUNT generation/validation (qmk#19515)

    * Add *_MATRIX_LED_COUNT parsing/validation

    * Disable parsing for now

    * Disable complexity check

commit 0a3ec7f
Author: Joel Challis <git@zvecr.com>
Date:   Thu Jun 1 21:12:25 2023 +0100

    Merge upstream uf2conv.py changes (qmk#21107)

commit a4ed6ad
Author: Ryan <fauxpark@gmail.com>
Date:   Fri Jun 2 02:25:08 2023 +1000

    Unicodemap keycodes rename (qmk#21092)

commit 45c9bc4
Merge: 857e9b7 b110a09
Author: QMK Bot <hello@qmk.fm>
Date:   Thu Jun 1 14:11:31 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit b110a09
Author: DeskDaily <65656486+DeskDaily@users.noreply.github.com>
Date:   Thu Jun 1 22:10:47 2023 +0800

    [Keyboard] Add lightweight65 keyboard (qmk#21034)

    Co-authored-by: Neil Brian Ramirez <nightlykeyboards@gmail.com>
    Co-authored-by: Neil Brian Ramirez <nightlyboards@gmail.com>

commit 857e9b7
Merge: 6156972 c805c10
Author: QMK Bot <hello@qmk.fm>
Date:   Thu Jun 1 09:26:41 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit c805c10
Author: Sergi Meseguer <zigotica@gmail.com>
Date:   Thu Jun 1 11:25:49 2023 +0200

    [Keymap] z12 zigotica keymap tweaks (qmk#20990)

commit 0c9c4a4
Author: adiabatic <adiabatic@users.noreply.github.com>
Date:   Thu Jun 1 02:25:33 2023 -0700

    [Keymap] `zweihander-macos`: Don’t pretend to be a mouse (qmk#20997)

commit 6156972
Merge: 5a26e5e 81bc092
Author: QMK Bot <hello@qmk.fm>
Date:   Wed May 31 19:03:39 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit 81bc092
Author: kkokdae <kkokdae@me.com>
Date:   Thu Jun 1 04:02:58 2023 +0900

    [Keymap] Modify kkokdae keymap for keyboardio/atreus (qmk#21037)

commit 5a26e5e
Author: 3araht <69518343+3araht@users.noreply.github.com>
Date:   Thu Jun 1 03:48:56 2023 +0900

    [Keymap] transpose added to giabalanai keymaps (qmk#21054)

commit 6a2de56
Merge: 04719c7 c2ddd77
Author: QMK Bot <hello@qmk.fm>
Date:   Wed May 31 18:47:46 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit c2ddd77
Author: CoffeeIsLife <36961653+CoffeeIsLife87@users.noreply.github.com>
Date:   Wed May 31 13:46:59 2023 -0500

    [Keymap] Cleanup coffeeislife87 keymap and remove features (qmk#21061)

    Co-authored-by: Drashna Jaelre <drashna@live.com>
    Co-authored-by: Fae <faenkhauser@gmail.com>

commit 04719c7
Author: Evgenii Vilkov <zzeneg@gmail.com>
Date:   Wed May 31 20:46:03 2023 +0200

    Fix backlight sync on suspend_power_down for split keyboards (qmk#21079)

commit b3a7f80
Merge: cc11b63 3a3e5ab
Author: QMK Bot <hello@qmk.fm>
Date:   Wed May 31 18:44:47 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit 3a3e5ab
Author: Drashna Jaelre <drashna@live.com>
Date:   Wed May 31 11:44:06 2023 -0700

    [Keymap] Drashna Keymap updates for 0.21.0 (qmk#21073)

commit cc11b63
Merge: 23658cf 1411c79
Author: QMK Bot <hello@qmk.fm>
Date:   Tue May 30 18:25:04 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit 1411c79
Author: Sergey Vlasov <sigprof@gmail.com>
Date:   Tue May 30 21:24:19 2023 +0300

    `qmk find`: Fix handling of functions in filters (qmk#21090)

    Functions in filters did not work properly except when used in the last
    (or only) filter.  The problem was caused by the peculiarity of the
    `lambda` behavior in Python — any variables from the outer scope are
    captured only by reference, therefore any subsequent reassignment of
    those variables is propagated to all lambdas created earlier in the same
    scope.  Together with the laziness of `filter()` (it returns an iterator
    which performs filtering on demand) this resulted in all function
    filters using the values of the `key` and `value` variables which
    correspond to the last filter in the sequence, therefore the result of
    filtering was wrong if some filter with a function was not the last one
    in the sequence.

    Apparently the shortest way to make a Python lambda capture some
    variables by value is to add arguments with default values for such
    variables (default values are evaluated when the lambda is created, and
    any subsequent reassignments in the outer scope no longer changes them).
    This makes filters with functions work properly even when such filters
    are not at the last position in the sequence.

commit 23658cf
Merge: ffeaf46 913691b
Author: QMK Bot <hello@qmk.fm>
Date:   Tue May 30 01:11:34 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit 913691b
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue May 30 02:10:57 2023 +0100

    Bump tj-actions/changed-files from 35 to 36 (qmk#21058)

    Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 35 to 36.
    - [Release notes](https://github.com/tj-actions/changed-files/releases)
    - [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md)
    - [Commits](tj-actions/changed-files@v35...v36)

    ---
    updated-dependencies:
    - dependency-name: tj-actions/changed-files
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit ffeaf46
Merge: 0ffa4ef 1e2dedd
Author: QMK Bot <hello@qmk.fm>
Date:   Tue May 30 01:08:54 2023 +0000

    Merge remote-tracking branch 'origin/master' into develop

commit 1e2dedd
Author: precondition <57645186+precondition@users.noreply.github.com>
Date:   Tue May 30 03:08:15 2023 +0200

    Remove outdated remarks regarding the default MT behavior (qmk#21077)
coquizen pushed a commit to coquizen/qmk_firmware that referenced this pull request Jun 22, 2023
Functions in filters did not work properly except when used in the last
(or only) filter.  The problem was caused by the peculiarity of the
`lambda` behavior in Python — any variables from the outer scope are
captured only by reference, therefore any subsequent reassignment of
those variables is propagated to all lambdas created earlier in the same
scope.  Together with the laziness of `filter()` (it returns an iterator
which performs filtering on demand) this resulted in all function
filters using the values of the `key` and `value` variables which
correspond to the last filter in the sequence, therefore the result of
filtering was wrong if some filter with a function was not the last one
in the sequence.

Apparently the shortest way to make a Python lambda capture some
variables by value is to add arguments with default values for such
variables (default values are evaluated when the lambda is created, and
any subsequent reassignments in the outer scope no longer changes them).
This makes filters with functions work properly even when such filters
are not at the last position in the sequence.
autoferrit pushed a commit to SpaceRockMedia/bastardkb-qmk that referenced this pull request Dec 8, 2023
Functions in filters did not work properly except when used in the last
(or only) filter.  The problem was caused by the peculiarity of the
`lambda` behavior in Python — any variables from the outer scope are
captured only by reference, therefore any subsequent reassignment of
those variables is propagated to all lambdas created earlier in the same
scope.  Together with the laziness of `filter()` (it returns an iterator
which performs filtering on demand) this resulted in all function
filters using the values of the `key` and `value` variables which
correspond to the last filter in the sequence, therefore the result of
filtering was wrong if some filter with a function was not the last one
in the sequence.

Apparently the shortest way to make a Python lambda capture some
variables by value is to add arguments with default values for such
variables (default values are evaluated when the lambda is created, and
any subsequent reassignments in the outer scope no longer changes them).
This makes filters with functions work properly even when such filters
are not at the last position in the sequence.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli qmk cli command python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants