-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
implementing random bookmark(s) selection #779
Conversation
@@ -461,7 +462,7 @@ class BookmarkVar(NamedTuple): | |||
def taglist(self) -> List[str]: | |||
return [x for x in self.tags_raw.split(',') if x] | |||
|
|||
bookmark_vars = lambda xs: (BookmarkVar(*x) for x in xs) | |||
bookmark_vars = lambda xs: ((x if isinstance(x, BookmarkVar) else BookmarkVar(*x)) for x in xs) |
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.
Avoid replacing already converted records (this helps with export_on
testing)
438a7ce
to
1072ea3
Compare
@@ -2186,12 +2189,13 @@ class BukuDb: | |||
LOGERR('No matching index %s', index) | |||
return False | |||
|
|||
single_record = len(results) == 1 |
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.
bugfix: when invoking with a set of indices, JSON output only printed 1 record before
else: | ||
print_json_safe(results, True, self.field_filter) | ||
print_json_safe(results, single_record, field_filter=self.field_filter) |
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.
…Technically this block can be removed along with return True
(and the single_record
thing moved to the near-identical piece of code below); but I wasn't sure if you'd accept changing output format when a single-value range is supplied 😅
@@ -2327,14 +2331,14 @@ class BukuDb: | |||
raise ValueError("Original tag cannot contain delimiter ({}).".format(DELIM)) | |||
|
|||
orig = delim_wrap(orig) | |||
newtags = parse_tags([DELIM.join(new)]) | |||
newtags = taglist_str(DELIM.join(new)) |
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.
Same thing but in a less roundabout way
|
||
if orig == newtags: | ||
raise ValueError("Original and replacement tags are the same.") | ||
|
||
# Remove original tag from DB if new tagset reduces to delimiter | ||
if newtags == DELIM: | ||
if not self.delete_tag_at_index(0, orig): | ||
if not self.delete_tag_at_index(0, orig, chatty=self.chatty): |
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.
bugfix: should be invoked with chatty=False
when in library mode
print('No bookmarks added yet ...') | ||
return False | ||
|
||
index = result[0] | ||
index = random.randint(1, max_id) |
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.
All other places use Python random access, so I updated this one as well
@@ -2596,12 +2600,15 @@ class BukuDb: | |||
if self._to_export is not None: | |||
_resultset = dict(old) | |||
_resultset.update({x.url: x for x in resultset if x.url in old}) | |||
resultset = list(_resultset.values()) | |||
resultset = self._sort(_resultset.values(), order) |
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.
bugfix: reapplying sorting
self._to_export = None | ||
if not resultset: | ||
print('No records to export') | ||
return False | ||
|
||
if pick and pick < len(resultset): | ||
resultset = self._sort(random.sample(resultset, pick), order) |
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.
Subset selection when using --export
with --random
@@ -4658,9 +4667,10 @@ def prompt(obj, results, noninteractive=False, deep=False, listtags=False, sugge | |||
pass | |||
return | |||
|
|||
skip_print = False |
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.
When combined with R
, printing out the next page should be skipped once.
1072ea3
to
b8ce3d9
Compare
if new_results or nav == 'n': | ||
count = 0 | ||
if (new_results or nav == 'n') and not skip_print: | ||
count = next_index |
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.
It makes more sense for the printed search result indices (31.
, 32.
, 33.
) to match the page indices (31-40/55
)
@@ -4674,8 +4684,11 @@ def prompt(obj, results, noninteractive=False, deep=False, listtags=False, sugge | |||
print('%d-%d/%d' % (cur_index + 1, next_index, total_results)) | |||
else: | |||
print('No more results') | |||
new_results = False |
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.
This message should not keep reappearing after all pages got printed.
@@ -4852,7 +4876,7 @@ def prompt(obj, results, noninteractive=False, deep=False, listtags=False, sugge | |||
if index < 0 or index >= count: | |||
print('No matching index %s' % nav) | |||
continue | |||
browse(results[index + cur_index][1]) | |||
browse(results[index][1]) |
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.
This matches up with the earlier change (cur = next_index
)
Also the check just above clearly expects index
to be between 1 and count
😅
@@ -6282,6 +6308,10 @@ POSITIONAL ARGUMENTS: | |||
|
|||
update_search_results = False | |||
if search_results: | |||
if args.random and args.random < len(search_results): | |||
search_results = bdb._sort(random.sample(search_results, args.random), order) | |||
single_record = args.random == 1 # matching print_rec() behaviour |
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.
Currently single_record
is tied to --random
.
(This can be changed to always using single record mode when only one is found.)
bdb.close_quit(1) | ||
if args.random and args.random < len(id_range): | ||
bdb.print_rec(random.sample(id_range, args.random), order=order) | ||
elif not args.print: |
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.
--random
+ --print
@@ -6423,7 +6449,7 @@ POSITIONAL ARGUMENTS: | |||
|
|||
# Export bookmarks | |||
if args.export and not search_opted and not export_on: | |||
bdb.exportdb(args.export[0], order=order) | |||
bdb.exportdb(args.export[0], order=order, pick=args.random) |
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.
args.random
is None
when --random
is not used
@@ -117,7 +116,7 @@ def shell_context(): | |||
"""Shell context definition.""" | |||
return {'app': app, 'bukudb': bukudb} | |||
|
|||
app.jinja_env.filters['netloc'] = lambda x: urlparse(x).netloc # pylint: disable=no-member | |||
app.jinja_env.filters.update(util.JINJA_FILTERS) |
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.
Moved Jinja filters into a separate file.
{% endif %} | ||
<a href="{{ url_for('bookmark.details_view', id=model.id, url=request.args.url) }}">{{_gettext('View Record')}} #{{model.id}}</a> | ||
</h3> | ||
{% endblock %} |
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.
Overriding default View Record header.
Random mode is enabled by providing random
as requested record ID.
@@ -6,9 +6,22 @@ | |||
{{ buku.close_if_popup() }} | |||
{% endblock %} | |||
|
|||
{% block model_menu_bar_before_filters %} | |||
{{ super() }} | |||
{% if data %} |
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.
The Random button is not displayed when the list is empty (to avoid dealing with error 404)
$(document).on('click', `#modal-random`, function() { | ||
$(`#fa_modal_window .modal-content`).load($(`#random`).attr('href')); | ||
}); | ||
</script> |
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.
Implementing “Pick another” here (this code is only run once so there's no danger of a livelock/memory leak).
import re | ||
|
||
get_netloc = lambda x: urlparse(x).netloc # pylint: disable=no-member | ||
select_filters = lambda args: {k: v for k, v in args.items() if re.match(r'flt.*_', k)} |
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.
A |flt
Jinja filter to prevent copying non-filter params into the random entry URL.
|
||
def sorted_counter(keys, *, min_count=0): | ||
data = Counter(keys) | ||
return Counter({k: v for k, v in sorted(data.items()) if v > min_count}) |
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.
Moved these two utils here as they seemed fairly general-purpose.
if self._filters: | ||
flt = self._filters[idx] | ||
models = list(flt.apply(models, flt.clean(value))) | ||
return models |
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.
Moved out duplicate code into a mixin
@@ -269,7 +273,10 @@ def get_list(self, page, sort_field, sort_desc, _, filters, page_size=None): | |||
bookmarks = bukudb.searchdb(keywords, order=order, **kwargs) | |||
else: | |||
bookmarks = bukudb.get_rec_all(order=order) | |||
bookmarks = self._apply_filters(bookmarks or [], filters) | |||
return self._apply_filters(bookmarks or [], filters) |
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.
Moved search out of get_list()
(to use it in random entry selection code).
bookmark = self.model.bukudb.get_rec_by_id(id) | ||
if bookmark is None: | ||
if id == 'random': | ||
bookmarks = self._from_filters(self._get_list_filter_args()) |
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.
Based on the filters
value passed to get_list()
in the upstream code
@@ -506,7 +508,7 @@ def get_pk_value(self, model): | |||
|
|||
def get_one(self, id): | |||
tags = self.all_tags[1] | |||
tag_sns = types.SimpleNamespace(name=id, usage_count=tags[id]) | |||
tag_sns = types.SimpleNamespace(name=id, usage_count=tags.get(id, 0)) |
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.
Fixing a (rarely occurring) error
self.bukudb.replace_tag(original_name, [model.name]) | ||
names = {s for s in re.split(r'\s*,\s*', model.name.lower().strip()) if s} | ||
assert names, 'Tag name cannot be blank.' # deleting a tag should be done via a Delete button | ||
self.bukudb.replace_tag(original_name, names) |
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.
The required
attribute cannot detect ,
as empty input (or ,, ,,
for that matter).
And if the user wanted to delete a tag, he'd be using the corresponding button.
mock.call.open().__enter__().write(_converted['data']), # pylint: disable=unnecessary-dunder-call | ||
mock.call.print('42 exported'), | ||
mock.call.open().__exit__(None, None, None)] | ||
assert wrap.mock_calls == ([] if not picked else pick_expected) + expected_calls |
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.
Testing behaviour of random parameter (pick=
) in export.
(It's named differently to avoid name collision with the module.)
@@ -144,7 +144,7 @@ def _test_add(bdb, prompt, *, add_tags=[], tag=[], tags_fetch=True, tags_in=None | |||
@pytest.mark.parametrize('count', [{}, {'count': ['10']}]) | |||
@pytest.mark.parametrize('order, indices, command', [ | |||
(['tags', '-url'], None, {'order': ['tags,-url'], 'print': []}), | |||
(['-description', '+uri'], {5, 8, 9, 10, 11, 12}, | |||
(['-description', '+uri'], [5, 8, 9, 10, 11, 12], |
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.
random.sample()
doesn't work on sets (though it works on ranges); so I'm converting the parsed IDs from the input to a list now.
@mock.patch('buku.format_json', return_value='formatted') | ||
@mock.patch('buku.print_json_safe') | ||
def test_random(_print_json_safe, _format_json, _write_string_to_file, _print_rec_with_filter, _sample, | ||
bdb, stdin, prompt, random, indices, json): |
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.
Testing --random
handling when combined with --print
, search and/or --json
@pytest.mark.parametrize('random', [None, 1, 3]) | ||
@mock.patch('random.sample', return_value='sampled') | ||
@mock.patch('buku.print_rec_with_filter') | ||
def test_random_export(_print_rec_with_filter, _sample, bdb, stdin, prompt, random, search): |
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.
Testing --export
separately (this simplifies the code a bit, and combining it with --print
is not supported anyway).
Thank you! |
Ability to poll for a random bookmark (without immediately opening it in a browser).
Usecase: “what do I read next?” (+bukuserver)
Screenshots
CLI
(can be used with print, search and export)
interactive shell
(
R
shows a random result,R 3
– 3 results,R -2
shows 2 random records ignoring last/current search)webUI
(the Random button searches within currently viewed list)
(the "Pick another" button is only present when viewing a random entry; it refreshes the modal contents which rerolls the ID)
(the View Record header link is identical to the one used in "successful update" message; it's handy if you want to edit a [random] entry, and then switch back to the list you were viewing)