Conversation
tremby
left a comment
tremby
left a comment
There was a problem hiding this comment.
Honestly I was prepared on clicking the link to this PR to say "nope I want to keep it simple" but then I looked at the changeset and this is very simple.
I like it.
I think I would prefer to make this optional, however. I think it should be opt-in, via an environment variable. Maybe if IMGUR_LOG_FILE is set, log it to that file, and so at the same time we let the user choose where. That also takes the responsibility for making sure that file doesn't end up ridiculously huge off our plate. (A user who uses this a lot might choose to make some log rotating daemon take care of it, for example.)
I've added a couple of comments.
In addition, please add something to the readme about this. I can do this if you don't want to.
After you've addressed them, please squash your commits (or I can do it for you if you prefer); I don't want the parts of the commits which undo and redo the recent changes, which I think must have crept in when you rebased.
Thanks!
imgur.sh
Outdated
| echo $url | ||
| echo "Delete page: https://imgur.com/delete/$delete_hash" >&2 | ||
| delete_page="https://imgur.com/delete/$delete_hash" | ||
| echo $url | sed 's/^http:/https:/' |
There was a problem hiding this comment.
This looks like a merge error; the sed has crept back in. Please undo that.
Set delete_page right after delete_hash gets set, then echo the URL, then echo the delete page to stderr.
There was a problem hiding this comment.
I only set $delete_page because it's used again below in logging; I do this for every variable used more than once. The logging echo could perfectly use "https://imgur.com/delete/$delete_hash" instead. Opinion?
There was a problem hiding this comment.
No strong opinion. The only important thing to me here was to not let sed creep back in, which would be a regression. Then I was dictating the order of lines but I don't feel that strongly about it. I wasn't complaining about making an extra variable -- I see why you did that and it makes sense.
| if [ $# -gt 0 ]; then | ||
| clip+=$'\n' | ||
| fi | ||
| # Keep a log of the uploads |
There was a problem hiding this comment.
Small nitpick, but please add a blank line before this.
imgur.sh
Outdated
| clip+=$'\n' | ||
| fi | ||
| # Keep a log of the uploads | ||
| echo "$(date +"%Y/%m/%d_%H:%M:%S") $file $url $delete_page" >> $HOME/.imgur.sh.log |
There was a problem hiding this comment.
-
Let's wrap this in a conditional.
if [ -n "${IMGUR_LOG_FILE-}" ]; thenshould do it. (That trailing dash is on purpose.)
-
Then append it to that file. (Nitpick: I like no space between the
>>and the file.) -
Please also use the ISO date format (
date -Isecond), and use a tab character between each pair of columns.
I'm not sure how valuable $file is, but short of checking whether it's stdin or a URL or a file and maybe running realpath on it, I don't know what to do. Probably just keep it simple and leave it how you are logging it.
There was a problem hiding this comment.
Conditional and date format done, although in my local copy I kept the original date format as -Isecond makes the date string unnecessarily long and short width terminals will wrap the output earlier. I also re-ordered the variables to print $file at the end, as this makes the columns nicely justified instead of stair-cased.
|
I'm not well versed in git, I'm an old-timer svn fella, so I'm not sure how to correctly make the changes appear in this PR; shall I create a new PR instead after you agree or not to my comments above? |
Just push the new commits, which you presumably made on the same local branch, to the same remote branch you pushed to before, and they'll appear here. |
|
In other words, probably just |
|
Sorry for the long absence, been very busy lately. I got this in my personal logs: It's obviously very rare. I must also clarify here that I only saw this in the log as I feed the output of some screen grabber directly to imgur.sh. My workaround is a match test just after the first echo |
2 participants
One might need to delete something later on