Skip to content

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

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

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

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
439ca8d
fix issues when selection is entire section but filter function expec…
jpenney Apr 1, 2022
cb5ef38
suggestion from review feedback
jpenney May 4, 2022
0bb0a6f
suggestion from review feedback
jpenney May 4, 2022
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
17 changes: 10 additions & 7 deletions scripts/sub-digest/subdigest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ def _find_line_expr(self, expr):

def _get_selection(self):
if self.selection is None:
return self._get_section()
# return copy of section contents as list
return list(self._get_section())

return [line for i, line in enumerate(self._get_section())
if i in self.selection]
Expand All @@ -130,15 +131,17 @@ def _get_nonselection(self):
if i not in self.selection]

def _process_selection(self, f):
lines = self._get_selection()

if self.selection is None:
f(self._get_section())
indices = range(len(lines))
else:
indices = sorted(self.selection)
lines = self._get_selection()
f(lines)
section = self._get_section()
for i, line in zip(indices, lines):
section[i] = line

f(lines)
Copy link
Copy Markdown

@FichteFoll FichteFoll Feb 12, 2023

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.

Copy link
Copy Markdown

@FichteFoll FichteFoll Feb 12, 2023

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.

section = self._get_section()
for i, line in zip(indices, lines):
section[i] = line

@filter
def use_styles(self) -> Subtitles:
Expand Down