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

Enhance install script to handle commented and uncommented lines (#3632) #4112

Merged
merged 3 commits into from
Dec 1, 2024

Conversation

buttering
Copy link
Contributor

Enhance install script to handle commented and uncommented lines in shell file with user prompts for modification.

  • Track commented and uncommented lines in the file.
  • Prompt user to append or skip if the line is commented.
  • Ensure new lines are added only when necessary, based on user input.
  • To the fish_user_key_bindings.fish, the original logic would append the line to the end if no corresponding statement was found. I’ve adopted the same behavior for commented lines.

This is my first contribution to this open-source project, and I’m really excited to get involved.
I saw the discussion in issue #3632 and wanted to try implementing the proposed feature.
One concern I have is whether I should write test cases for this change. The existing Ruby tests seem not to support testing the fzf installation process directly. Additionally, I worry that the logic in the updated code might be a bit complex (though I’ve tried my best to keep it readable).
Please feel free to provide feedback on my changes. If there’s anything I can improve, I’d greatly appreciate your guidance.

…egunn#3632)

Resolves junegunn#3632
Enhance install script to handle commented and uncommented lines in shell file with user prompts for modification.
- Track commented and uncommented lines in the file.
- Prompt user to append or skip if the line is commented.
- Ensure new lines are added only when necessary, based on user input.
- To the `fish_user_key_bindings.fish`, the original logic would append the line to the end if no corresponding statement was found. I’ve adopted the same behavior for commented lines.
@junegunn
Copy link
Owner

junegunn commented Nov 30, 2024

Thanks for your interest in this project. If we're going to display the matching lines, we can just use the output of the grep comment.

Here's my take on the problem. What do you think?

diff --git a/install b/install
index 5b67e30..607c7ef 100755
--- a/install
+++ b/install
@@ -295,35 +295,43 @@ EOF
 fi
 
 append_line() {
-  set -e
-
-  local update line file pat lno
+  local update line file pat lines
   update="$1"
   line="$2"
   file="$3"
   pat="${4:-}"
-  lno=""
+  lines=""
 
   echo "Update $file:"
   echo "  - $line"
   if [ -f "$file" ]; then
     if [ $# -lt 4 ]; then
-      lno=$(\grep -nF "$line" "$file" | sed 's/:.*//' | tr '\n' ' ')
+      lines=$(\grep -nF "$line" "$file")
     else
-      lno=$(\grep -nF "$pat" "$file" | sed 's/:.*//' | tr '\n' ' ')
+      lines=$(\grep -nF "$pat" "$file")
     fi
   fi
-  if [ -n "$lno" ]; then
-    echo "    - Already exists: line #$lno"
-  else
-    if [ $update -eq 1 ]; then
-      [ -f "$file" ] && echo >> "$file"
-      echo "$line" >> "$file"
-      echo "    + Added"
-    else
-      echo "    ~ Skipped"
+  if [ -n "$lines" ]; then
+    echo "    - Already exists:"
+    sed 's/^/        Line /' <<< "$lines"
+
+    update=0
+    if [ $(\grep -vn "^[0-9]*:[[:space:]]*#" <<< "$lines" | wc -l) -eq 0 ]; then
+      echo "    - But they all seem to be commented"
+      ask "    - Continue modifying $file?"
+      update=$?
     fi
   fi
+
+  set -e
+  if [ $update -eq 1 ]; then
+    [ -f "$file" ] && echo >> "$file"
+    echo "$line" >> "$file"
+    echo "    + Added"
+  else
+    echo "    ~ Skipped"
+  fi
+
   echo
   set +e
 }
Update /Users/jg/.bashrc:
  - [ -f ~/.fzf.bash ] && source ~/.fzf.bash
    - Already exists:
        Line 520:[ -f ~/.fzf.bash ] && source ~/.fzf.bash # This one is NOT commented
        Line 521:# [ -f ~/.fzf.bash ] && source ~/.fzf.bash # This one is commented
    ~ Skipped
Update /Users/jg/.bashrc:
  - [ -f ~/.fzf.bash ] && source ~/.fzf.bash
    - Already exists:
        Line 520:# [ -f ~/.fzf.bash ] && source ~/.fzf.bash # This one is commented
        Line 521:  # [ -f ~/.fzf.bash ] && source ~/.fzf.bash # This one is also commented
    - But they all seem to be commented
    - Continue modifying /Users/jg/.bashrc? ([y]/n) n
    ~ Skipped

@junegunn
Copy link
Owner

One concern I have is whether I should write test cases for this change. The existing Ruby tests seem not to support testing the fzf installation process directly.

No, we currently don't test install script. So don't worry about it. The importance of the script has declined since most users use a package manager to install fzf, and setting up shell integration is much easier now than it used to be, and it's feasible to manually add a single line to your shell configuration file.

https://github.com/junegunn/fzf/blob/master/README.md#setting-up-shell-integration

- Replaced `lno` variable with `lines` to store matching lines and simplified the logic.
- Improved line existence check, now prints all matching lines directly and handles commented lines separately.
- Removed unnecessary variables like `all_commented`, `commented_lines`, and `non_commented_lines`.
@buttering
Copy link
Contributor Author

I’ve learned a lot about Shell scripting from your explanation. I truly admire the elegance and simplicity of your code.

@junegunn junegunn merged commit ac508a1 into junegunn:master Dec 1, 2024
5 checks passed
@junegunn
Copy link
Owner

junegunn commented Dec 1, 2024

Merged, thanks for taking the initiative!

@buttering buttering deleted the comment branch December 2, 2024 08:19
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