Skip to content

Added a few features and tried to make it shellcheck compliant#22

Closed
txtyash wants to merge 2 commits intotremby:mainfrom
txtyash:main
Closed

Added a few features and tried to make it shellcheck compliant#22
txtyash wants to merge 2 commits intotremby:mainfrom
txtyash:main

Conversation

@txtyash
Copy link

@txtyash txtyash commented Feb 8, 2022

Curl's progress bar could look ugly to some people. I added it anyways.

I think instead of using global variables, the program should prompt the user.

I've tested the new functions I added and seem to work fine.

Couldn't fix this part for shell compliance:

[Line 147:](javascript:setPosition(147, 10))
    if [ $? -ne 0 ]; then
         ^-- [SC2181](https://github.com/koalaman/shellcheck/wiki/SC2181) (style): Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?.

Also didn't try to fix(shellcheck) the global variables for copying to clipboard because I think you should use a prompt instead. Can I work on that?

@tremby
Copy link
Owner

tremby commented Feb 18, 2022

Sorry, but personally I don't think I see a point to much of this.

  • I like logging to a file but I think this was done better in Add upload history log #23.

  • I don't see a point in listing out that log file. I think any user of this script knows about tail, cat, etc.

  • Likewise I don't see much point in the --open or --remove features. These seem trivial to achieve manually by looking at the log, or right after uploading with firefox $(xsel) or similar.

  • The progress bar -- I considered it in the past. Sure, it shows progress for the upload, which is nice if you're uploading something huge or on a very slow connection, but then it sits at 100% for some unpredictable amount of time while imgur processes your image. In my case that's invariably way longer than the upload took. I don't think it's useful.

  • You've changed from echo to printf, and I have no idea why.

  • Your changeset almost doubles the length of the script.

  • Then there's shellcheck, which I'd never seen before.

    I just tried it, and a lot of its output looks like nonsense to me.

    Run on my own current version of the script:

    In imgur line 17:
    	echo "Usage: $(basename $0) [<filename|URL> [...]]" >&2
                                    ^-- SC2086: Double quote to prevent globbing and word splitting.
    

    Yeah, fair enough.

    In imgur line 38:
    if [ "$1" == "-h" -o "$1" == "--help" ]; then
                      ^-- SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
    

    etc

    Ehh. It points to https://www.shellcheck.net/wiki/SC2166 whose only complaint only matters if we're starting arguments with - or using !, and we're doing neither here. We use a ! later but I don't see how it could be misinterpreted in that case. I guess I could be persuaded.

    In imgur line 75:
    	if [ $? -ne 0 ]; then
                 ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
    

    Just like you seem to have concluded, this seems like nonsense. The command was necessarily run earlier, in different ways in two branches of a conditional.

    In imgur line 92:
    	echo $url
                 ^--^ SC2086: Double quote to prevent globbing and word splitting.
    

    Fair enough I guess but it's a URL from a trusted source; there shouldn't be anything weird in there.

    In imgur line 104:
    	echo -n "$clip" | pbcopy $IMGUR_PBCOPY_OPTIONS
                                     ^-------------------^ SC2086: Double quote to prevent globbing and word splitting.
    
    
    Did you mean: 
    	echo -n "$clip" | pbcopy "$IMGUR_PBCOPY_OPTIONS"
    

    No I did not. I intentionally want word splitting here.

    In imgur line 107:
    elif [ $DISPLAY ]; then
           ^------^ SC2086: Double quote to prevent globbing and word splitting.
    
    Did you mean: 
    elif [ "$DISPLAY" ]; then
    

    Ehhh I don't see why anyone would put something weird in $DISPLAY but sure whatever. I should probably be doing an -n test here actually.

  • Finally,

      NAME 
      Randomly select & apply an kitty color theme
    

    Ooh, I wish.

As a suggestion for PRs you may open in the future, on any project, try to keep each PR to a single narrow topic -- this was too broad.

By all means maintain a fork if you like -- I think it's great that you're adding features which make it function the way you want it to -- but I won't merge this. I may at some point apply some of shellcheck's suggested changes.

@tremby tremby closed this Feb 18, 2022
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