Aggregated Logbook #41
Conversation
added bids_logbook_entries_agg to get an aggregated list of ascent and tries.
src/boardlib/api/aurora.py
Outdated
| @@ -580,5 +581,115 @@ def logbook_entries(board, username, password, grade_type="font", db_path=None): | |||
|
|
|||
| return full_logbook_df | |||
|
|
|||
| def bids_logbook_entries_agg(board, token, user_id, db_path=None): | |||
There was a problem hiding this comment.
My suggestion in the other PR was that we could refactor that PR to adjust the params to get the output needed for the Climbdex changes. It looks like this PR just duplicates everything. E.g. we could refactor bids_logbook_entries to take a token instead of user/password and we can parametrize which columns are yielded to get the additional columns that this PR adds.
Seems like there is a lot of repeated code here that will be difficult to maintain in the future. Let me know if I'm missing something here and there is an issue with both features using the same underlying functions with slightly different parameters instead of duplicated functions with slightly altered functionality.
There was a problem hiding this comment.
makes sense, we tried our best to refactor the function, see comment below.
Refactored logbook_entries function to allow console csv generation and generation of the aggregated files for climbdex within one function
|
We refactored the function so the command To get the aggregated version of the logbook, which is used to calculate bids/attempts we use the same function The refactor was a bit tricky, but all our tests worked. |
|
|
||
| entries = api.logbook_entries(board, username, password, grade_type, db_path=database, aggregate=False) | ||
| for entry in entries: | ||
| if isinstance(entry, dict): |
There was a problem hiding this comment.
Why do we need to add this check for these changes? When is entry not a dict?
| @@ -1,17 +1,16 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
There are a lot of changes in this file, but it seems to me like at most this should just be a one-line change to add aggregate=False, but really there shouldn't be any changes at all because aggregate is already defaulting to False. Can we revert this file please? There are multiple regressions, e.g. to the command output prints, the imports, and the styling.
|
|
||
| entry = { | ||
| "climb_uuid": raw_entry["climb_uuid"], | ||
| "board": board, |
There was a problem hiding this comment.
Why does board need to be in the entry? It's already passed as an argument, so the caller already knows what the board is.
|
|
||
| if aggregate: |
There was a problem hiding this comment.
It doesn't seem like we really need to add this if clause at all. It's such a minor performance gain to drop a few fields from the aggregate version and perhaps we'd eventually want all of these fields in both versions anyways (for example, comment seems like something we'd eventually display).
| user_id = login_info["user_id"] | ||
|
|
||
| bids_entries = list(bids_logbook_entries(board, username, password, db_path)) | ||
| def logbook_entries(board, username, password=None, token=None, user_id=None, grade_type="font", db_path=None, aggregate=False): |
There was a problem hiding this comment.
We don't need a function to support both forms of arguments. We should just have the command line call an extra function to fetch the token and then have logbook_entries require a token.
| displayed_grade = convert_difficulty_to_grade(difficulty, grades_dict, grade_type) | ||
| if aggregate: | ||
| entry = { | ||
| 'uid': f"{bid_row['climb_uuid']}-{bid_row['angle']}", |
There was a problem hiding this comment.
Let's stay consistent with uuid, confusing to drop the u here. If you want, it could also be climb_angle_uuid to be more descriptive. Maybe I'm missing something here, but couldn't Climbdex construct the climb + angle ID on that end so that boardlib didn't need to worry about this nuance?
| @@ -367,31 +365,42 @@ def get_bids_logbook(board, token, user_id): | |||
| sync_results = user_sync(board, token, user_id, tables=["bids"]) | |||
| return sync_results["PUT"].get("bids", []) | |||
|
|
|||
| def bids_logbook_entries(board, user_id, token, db_path=None, aggregate=False): | |||
There was a problem hiding this comment.
Can you explain what the goal is for the aggregate version? I thought the two changes needed for Climbdex were the UUID in the final output and the ability to use a token instead of username/password for auth. It seems like there is quite a bit more than this going on but it's a little hard to follow. I'm happy to make the changes as well if it seems cumbersome, just not sure exactly what differences are required.
There was a problem hiding this comment.
I was probably thinking way too complicated here and went far off the beaten track. I'll think this through one more time and may start from scratch. I guess we should just add climb-uuid to output, change the auth-method to token and move the data transforming/aggregation to climbdex … sorry that this got a bit complicated or messed up here :)
There was a problem hiding this comment.
Sounds good and no worries, happy to help if needed!
as discussed in lemeryfertitta/Climbdex#55
slightly updated version of
logbook_entries()functions from @PTschaikner to create an aggregated logbooklogbook_entries_agg()with all needed informations which are used to get personal stats about the each climb ('climb_uuid', 'climb_name', 'board', 'date', 'is_mirror', 'angle', 'tries', 'sessions' 'time_since').instead of the login credentials (user_id and password)
logbook_entries_agg()uses user_id and token to fetch the results.