any interest in resolving shellcheck warnings? I have PRs ready

aurora / rmate

Remote TextMate 2 implemented as shell script

GNU General Public License v3.0

887 stars 123 forks source link

any interest in resolving shellcheck warnings? I have PRs ready #54

Closed mobilemind closed 7 years ago

mobilemind commented 7 years ago

I've run the latest version of rmate-sh 0.9.9 (2017-04-06) through shellcheck v0.4.6 https://www.shellcheck.net/ and addressed all the warnings I could.

Mostly the warnings were about:

more conservative use of double quotes (assignments, args)
use of backticks for shell evaluation versus newer $(..) syntax
use of read without "-r" option [may result in mangling of backslashes]
creation of filepaths var as an array, but expansion & re-assignment as a string
not handling potential failed cd commands

I'll run things on my machine with the changes for a few days and then make pull requests -- UNLESS someone indicates they think resolving shellcheck warnings is ill advised right now.

aurora commented 7 years ago

Quite a lot of warnings ... i've no problem in improving the code, but i wonder if there might be false-positives, too ... so if you decide to open pull requests, it would be cool, if you could open individual pull-requests for each type of warning. Thanks!

mobilemind commented 7 years ago

Thanks, I posted PRs for the two simplest cases with the fewest lines of code changed. PR notes include filtered shellcheck output for convenience.

aurora commented 7 years ago

@mobilemind Thanks for the PRs! I am currently really busy again, but i would like to test this first before merging. Unfortunately i will not have time for this before 8th of may. Just wanted to let you know, in case you wonder why i didn't merge it yet.

mobilemind commented 7 years ago

I understand; side projects are worthy efforts, yet often show changes in bursts. Well catch up next month.

For now, I'll continue with my modified rmate script for daily use and see if issues pop up.

However, while frequent, the scope of my testing is light & limited— I typically:

connect to the same two servers (RedHat) over default ports,
use rmate filename (or fileglob) to start a Textmate session,
have short sessions of 2-5 minutes, rarely 30-45 minutes,
work with small, simple files (shell scripts, markdown, HTML, JavaScript; 1 KB - 8 KB)
very rarely pipe to rmate or use it in an $EDITOR -w style context.

I hope that helps you and other users/contributors.

aurora commented 7 years ago

Hi, sorry it took a little longer for me to start looking into your pull requests, but i hope that'll have more time for this within the next days ...

aurora commented 7 years ago

I am going to close this now. Please feel free to open more pull requests, if you like!