Conversation
Summary of ChangesHello @michen00, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a convenient new feature that allows users to quickly copy typographic em dashes (—) and en dashes (–) to their system clipboard using simple shell commands, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful utility for copying en and em dashes to the clipboard. The cross-platform support in the _mnn script is well-implemented. My main feedback concerns the implementation of the em_ and en_ wrapper scripts, which are currently fragile. The standard and most maintainable way to implement this pattern is to use symbolic links. I've provided an alternative suggestion to use robust wrapper scripts to avoid code duplication and improve maintainability. I've also included a few suggestions to simplify and improve the main _mnn script.
| select_clipboard_command() { | ||
| local platform=$1 | ||
| case "$platform" in | ||
| macos) | ||
| if command -v pbcopy >/dev/null 2>&1; then | ||
| echo "pbcopy" | ||
| else | ||
| echo "ERROR:pbcopy not found" | ||
| fi | ||
| ;; | ||
| x11) | ||
| if command -v xclip >/dev/null 2>&1; then | ||
| echo "xclip -selection clipboard" | ||
| elif command -v xsel >/dev/null 2>&1; then | ||
| echo "xsel --clipboard" | ||
| else | ||
| echo "ERROR:xclip or xsel not found" | ||
| fi | ||
| ;; | ||
| wayland) | ||
| if command -v wl-copy >/dev/null 2>&1; then | ||
| echo "wl-copy" | ||
| else | ||
| echo "ERROR:wl-copy not found" | ||
| fi | ||
| ;; | ||
| windows) | ||
| if command -v clip.exe >/dev/null 2>&1; then | ||
| echo "clip.exe" | ||
| else | ||
| echo "ERROR:clip.exe not found" | ||
| fi | ||
| ;; | ||
| *) | ||
| echo "ERROR:unsupported platform" | ||
| ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
Returning error messages as strings (e.g., ERROR:pbcopy not found) and checking for them with string comparisons is a viable but somewhat brittle approach. A more idiomatic and robust pattern in shell scripting, especially when using set -e, is to have functions report errors on stderr and return a non-zero exit code. The calling code can then rely on set -e to halt execution automatically on failure.
For example, select_clipboard_command could be refactored to echo errors to stderr and return 1. Then in main, you could simply call it and let the script exit if it fails.
| if [[ "$SCRIPT_NAME" == "_mnn" ]]; then | ||
| echo "Error: This script must be invoked as 'en_' or 'em_'" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This explicit check for _mnn is redundant. The select_dash function, which is called shortly after, already has a default *) case that handles any script name other than en_ or em_, including _mnn. You can safely remove this block to simplify the code.
| if [[ "$platform" == "windows" ]]; then | ||
| # Windows echo doesn't support -n flag | ||
| echo "$dash" | $clipboard_cmd || { | ||
| echo "Error: Failed to copy to clipboard: $?" >&2 | ||
| exit 1 | ||
| } | ||
| else | ||
| # Unix-like systems support echo -n | ||
| echo -n "$dash" | $clipboard_cmd || { | ||
| echo "Error: Failed to copy to clipboard: $?" >&2 | ||
| exit 1 | ||
| } | ||
| fi |
There was a problem hiding this comment.
Instead of using a platform-specific if/else to handle the -n flag for echo, you can use printf. The printf "%s" "$dash" command is more portable and behaves consistently across different shells and operating systems, including Windows environments like MSYS/Cygwin. This simplifies your code by removing the conditional block.
# Copy to clipboard
printf "%s" "$dash" | $clipboard_cmd || {
echo "Error: Failed to copy to clipboard: $?" >&2
exit 1
}
1 participant
Description
Have you ever Googled 'en dash' or 'em dash' just so you can copy and paste it somewhere? Say less. Push an en dash (U+2013) or an em dash (U+2014) onto your clipboard with single command:
em_for an em dash: —en_for an en dash: –Inspiration: https://github.com/rskottap/personal/blob/master/bin/emoji