Splitting should be posix on "darwin" (macOS)

maaaaz / webscreenshot

A simple script to screenshot a list of websites

GNU Lesser General Public License v3.0

653 stars 162 forks source link

Splitting should be posix on "darwin" (macOS) #41

Closed manuelbua closed 4 years ago

manuelbua commented 4 years ago

Matching the partial "win" word will also include "darwin", non-POSIX argument splitting will cause problems when spaces are encountered (such as "header=Cookie: name=value").

However i'm not sure what the original intent was here: you may wanted to include "darwin" for some reason.

I'm using the string "win32" here as it seems there are various ways to detect it: one such interesting post at StackOverflow suggest to use platform.system() so to check for "Windows" instead.

maaaaz commented 4 years ago

"Matching the partial "win" word will also include "darwin".

Oh shit, I never thought of this !

No intent to discard or include MacOS, my only goal is to support Linux and Windows as I don't have a MacOS machine to test: but if by some kind of fortunate magic, webscreenshot also works on MacOS, I'm glad.

If I replace :

This expression posix="win" not in sys.platform
By this one posix="win32" not in sys.platform

=> Will it work on MacOS ?

manuelbua commented 4 years ago

Yeah that will work too as well as i've just tried it now, i moved it to an is_windows variable only because i've seen it used at send_signal as well.

But now i see also craft_arg is checking that too with "win", (tested it now and "win32" also works there), maybe having a global is_windows is a better alternative?

maaaaz commented 4 years ago

You're right, I'll make a specific and unique function to detect Windows.

maaaaz commented 4 years ago

Fixed in v2.8