Auto-creating folders in logging path #2 #21
Conversation
…g auto-creating folders in logging path
|
Hello @bruno- ! Can you review my commit? |
logging.tmux
Outdated
| } | ||
|
|
||
| setup_clear_history_key_binding() { | ||
| local key="$(get_tmux_option "$clear_history_key_option" "$default_clear_history_key")" | ||
| tmux bind-key "$key" run-shell "$CURRENT_DIR/scripts/clear_history.sh" | ||
| tmux bind-key "$clear_history_key" run-shell "$CURRENT_DIR/scripts/clear_history.sh" |
There was a problem hiding this comment.
Given that all the above functions are just one-liners now, how do you feel about just having them in the main function?
There was a problem hiding this comment.
It's a good suggestion! I will make it in the second commit :)
| local name_template=$(get_tmux_option "$name_option" "$default_name") | ||
| tmux display-message -p "$name_template" | ||
| } | ||
|
|
scripts/toggle_logging.sh
Outdated
| "$CURRENT_DIR/start_logging.sh" "$filename" | ||
| display_message "Started logging to $filename" | ||
| local file=$(tmux display-message -p "$logging_full_filename") | ||
| local path=$(tmux display-message -p "$logging_path") |
There was a problem hiding this comment.
Seing these 2 lines with tmux display-message -p again tripped me, despite getting your clarification in #20.
Chances are anyone else reading the source will be puzzled as well. How about extracting tmux display-message -p to a helper function? The purpose would be just to clarify intention. Example names: expand_value, expand_tmux_formats.. or something more fitting
There was a problem hiding this comment.
I think about this part... Maybe you are right and extracting this code to external function will be a good decision
scripts/capture_pane_helpers.sh
Outdated
| local history_limit="$(tmux display-message -p -F "#{history_limit}")" | ||
| tmux capture-pane -J -S "-${history_limit}" -p > "$file" | ||
| elif [[ $capture_scope == "Screen capture" ]]; then | ||
| local file=$(tmux display-message -p "$screen_capture_full_filename") | ||
| local path=$(tmux display-message -p "$screen_capture_path") | ||
| mkdir -p "$path" |
There was a problem hiding this comment.
After your refactoring it seems as this function is unnecessarily doing 2 things: capturing history and doing screen capture.
Would it make things simpler if this one was split into two functions? With that we could get rid of this whole file because the functions would be kept in files where they're used..
There was a problem hiding this comment.
I agree it will be simpler than the current state. I will refactor this part
|
Good stuff! Thank you for your work on this 👍 I think this is ready to be merged. I left some comments, but none are deal-breaking. Let me know your thoughts! |
|
I think I will refactor some part of code based on your comments and after make new PR for it :) |
|
Hello, @bruno- ! I made the second commit with some optimization. Check it, please! |
|
@selfuryon thank you for the good work and patience! |
Hello!
I created PR after some discussion on #20. The main changes are the way which variables work. Reduced the ambiguous path and filename with many
get_tmux_option. So we don't needget_pathlike functions anymoreBased on this issue #19