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

fix issues when selection is entire section but filter function expects list #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jpenney
Copy link
Contributor

@jpenney jpenney commented Apr 1, 2022

Fixes issue with both sort_field and sort_expression, which are using functionality specific to list (others filters might as well). Current version fails to sort without a selection:

$ /tmp/subdigest.py -i test.ass --sort-field layer ASC
Traceback (most recent call last):
  File "/tmp/subdigest.py", line 569, in <module>
    sys.exit(main())
  File "/tmp/subdigest.py", line 557, in main
    sub_obj = filt(*filter_args)
  File "/tmp/subdigest.py", line 408, in sort_field
    self._process_selection(_sort)
  File "/tmp/subdigest.py", line 134, in _process_selection
    f(self._get_section())
  File "/tmp/subdigest.py", line 407, in _sort
    events.sort(key=lambda line: getattr(line, field), reverse=order == "DESC")
AttributeError: 'EventsSection' object has no attribute 'sort'

Changes:

  • Subtitles._get_selection()
    • always return a list
  • Subtitles._process_selection()
    • always call _get_selection() for items to process
    • single code path for reinserting changes

…ts a list

    - `Subtitles._get_selection()`: always return a list
    - `Subtitles._process_selection()`: always call `_get_selection()` for items to process
jpenney and others added 2 commits May 4, 2022 12:29
@jpenney jpenney requested a review from FichteFoll February 10, 2023 13:25
for i, line in zip(indices, lines):
section[i] = line

f(lines)

Choose a reason for hiding this comment

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

Reading this code properly, I'm convinced that _process_selection should call the provided callback with each line as the argument and not a list of lines so that it would be sortable since that would technically also allow modifying the list's size (by adding or removing entries) and the following code wouldn't handle that properly.

Sorting should then use a _sort_selection handler that accepts a key function as the parameter which will then be used on the list that is extracted similarly to how it happens in _process_selection currently because that is a still a decent way to implement this.

Now that's all about internal code, but I like clean APIs also for internal calls. It would also have likely made the bug being fixed here more obvious.

Choose a reason for hiding this comment

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

As a clarification, that's not meant as a request to change the PR. I was just rambling about it as I was reading the code. You're still free to implement it that way, of course, and split the two different kinds of usages of _process_lines.

Always returning a list in _get_selection may not be a bad thing either but it does have a tiny bit of overhead when it may not be needed most of the time.

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.

2 participants