-
Notifications
You must be signed in to change notification settings - Fork 191
Added main module detection and cli construction, now should show the… #361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Needs Python 2 support and a test, do you think you could add those? (I might be able to do the Py2 support if you add a test.) If a test is not possible, then at least a little example would help. |
|
Sorry, I'm not going to add them, I have no time for it. But I have tested that against my apps using plumbum.cli, they work as indended. If we strictly need tests, something like https://github.com/laffra/auger (an automatic tests generator) may be helpful. |
|
About python2: how 'bout dropping support of it? |
|
For a end-user library, sure; but for something like Plumbum it can't be done, at least for a few more years. Plumbum needs to work anywhere a shell script works, and by default, CentOS7 and macOS still only provide Python 2. And I also work for LHCb, and they are not supporting Python 3 for another year or two. Rewriting 1M+ lines of code is not easy. I can't even drop Python 2.6 support yet since it is default on SL6 😢 |
|
I'll look at adding tests then, but it will be a while before I have time. |
5452a47 to
ac5a391
Compare
ac5a391 to
6412f8a
Compare
6412f8a to
0224797
Compare
7f7ee83 to
689cc58
Compare
efad6a8 to
c120d2f
Compare
c120d2f to
fc874f6
Compare
fc874f6 to
e93c966
Compare
|
Wouldn't it have been easier to address the Py2 + test request than to maintain this as a branch for 3+ years? Anyway, Python 2 support is working with one small change. Currently the only problem is that this breaks the current tests on Windows - it seems it is replacing the program name with |
|
about import ctypes
import platform
impl = platform.python_implementation()
if impl == "cpython" or impl == "PyPy" :
ctypes.pythonapi.Py_GetArgcArgv.restype = None
ctypes.pythonapi.Py_GetArgcArgv.argtypes = [
ctypes.POINTER(ctypes.c_int),
ctypes.POINTER(ctypes.POINTER(ctypes.c_wchar_p))
]
def realArgv():
count = ctypes.c_int()
args = ctypes.pointer(ctypes.c_wchar_p())
ctypes.pythonapi.Py_GetArgcArgv(ctypes.byref(count), ctypes.byref(args))
argc = count.value
argv = [str(args[i]) for i in range(count.value)]
return argv
print(realArgv())but it is not a solution that can be used, since it would break for the impls without |
ae39984 to
6ea694f
Compare
6ea694f to
ee35a13
Compare
ee35a13 to
294c191
Compare
294c191 to
fe33539
Compare
fe33539 to
3961ced
Compare
3961ced to
e3b69ea
Compare
e3b69ea to
2941f29
Compare
2941f29 to
5542532
Compare
5542532 to
98b58e2
Compare
|
Could you add a test? I think it's safe to merge now that Py 2 is gone. |
|
Also you can use the original simpler solution, I think, now without Py 2. Is there still an issue on Windows? |
d23987e to
8958983
Compare
Seems to be doing the opposite on Windows? |
|
I usually don't use Windows, and I don't know when I use it. |
|
Okay. I tried to get this working briefly before release, then saw the regression on Windows and left it. I don't use Windows much either, I'm just going from the GHA CI (we do have automated tests here). We also need a test showing the "working" behavior so we can verify it works there too. I'll eventually try to come back by and see if I can fix it, then. |
… show the right name (instead of __main__.py) when called for modules Co-Authored-By: Henry Fredrick Schreiner <[email protected]>
8958983 to
9d340ee
Compare
… right name (instead of
__main__.py) when called for modules