Closed jyn514 closed 2 years ago
I will attempt to fix the PowerShell issues myself, since I have some experience with PS.
This looks good, but I would like justification for why we got rid of main()
. I also have some reservations about the implementation of the wrapper, but I don't really have a better idea yet.
Hmm, it looks like even with a4b3ae2, the wrapper methodology still does not work on Windows, unless I am reading the AppVeyor logs incorrectly.
Interestingly, tests besides --version succeed. No idea why --version
would give an error.
Logic behind removing main - this requires a bit of background, apologies if you know most of this.
In Python, it's customary to put 'main' routines in their own block, called at the end by if __name__ == "__main__"
. This allows imports to access the script programmatically, without having to worry about arguments; i.e. something like from bitshuffle import decode
would work fine.
Since we've already separated functions that can be imported into their own files (library
and __init__
), there's no reason to keep a guard for __name__
. Even disregarding style, __name__
is set to the name of the file if not run directly; since the file it's in is named __main__.py
, if __name__ == "__main__"
will always be true.
I agree this was probably not the place to make such a change, I'll revert it and propose it later.
@charlesdaniels may I ask what you dislike about the wrapper? It's important it be in python so that it keeps Windows compatibility; if you have a suggestion it would be welcome. Otherwise I thought I did it fairly reasonably.
The logic behind removing main() is sound and I agree.
It would be better if the wrapper could run without aubprocess it exec, by importing bishuffle and calling main. This could be accomplished outside of a pip using sys.path.append.
I will investigate the PS issue further later today when I’m not on mobile.
bitshuffle is dead, long live bitshuffle 🐍
This is a mess of a merge, sorry about that.
Highlights:
--input
and--output
have been removed completely as arguments. This simplifies logic considerably. Shell redirection (< > |
) can be used as an alternativestdin
is not a terminal and run without arguments, default to encoding. Solves https://github.com/charlesdaniels/bitshuffle/issues/55main
is no longer a function; since__name__
will always be__main__
, it would have always runinput
/output
with<
/>
was sufficient. The exceptions:Note also that Powershell tests are very broken and I don't know how to fix them. From the errors, it should be fairly simple, I just don't know powershell. This request should not be merged until the tests have been fixed.
Minor changes:
--decode
and--encode
are now mutually exclusive. Previously, if both were specified the message would be encoded--version
now usesargparse
's built-inversion
actionwrapper.py
no longer usesexecvp
; it was incompatible with shell redirection. This is not due to the change, it was just made very evident