-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Make module executable (python -m pystdf) to start the scripts #10
base: master
Are you sure you want to change the base?
Conversation
Also clean them up a bit, some were no longer working.
This allows to run any of the scripts ('txt', 'xml', 'xlsx', 'count', slice') by executing python -m pystdf txt data.stdf
Building a source distribution is now functional again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this PR. I really like the modernized approach here. I'd just wonder about breaking existing users who depend on the installed scripts. Seems like a major version is needed here.
Some notes below. I wouldn't blame you for not fixing these, if anything they're a reminder to myself how I might shepherd these changes in, if you're all out of patience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project really needs tests. There are a few STDF files lying around on the open internet we might add for a test suite.
conversion, file = sys.argv[1:3] | ||
args = sys.argv[3:] | ||
|
||
if conversion not in ['txt', 'xml', 'xlsx', 'slice', 'count']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these seem like format conversions (txt, xml, xlsx), slicing seems like an output option, and count seems like a summary function. Might be worth splitting these into different subcommands and option arguments.
GZ_PATTERN = re.compile('\.g?z', re.I) | ||
BZ2_PATTERN = re.compile('\.bz2', re.I) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GZ_PATTERN = re.compile('\.g?z', re.I) | |
BZ2_PATTERN = re.compile('\.bz2', re.I) | |
GZ_PATTERN = re.compile('\.g?z$', re.I) | |
BZ2_PATTERN = re.compile('\.bz2$', re.I) |
These are better names, just noticing I also forgot to anchor the file extension. Total driveby.
It might also be reasonable to support stdin in the CLI, so one could pipe uncompressed output directly, without relying on this tool to do everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this auto-uncompressing logic is repeated several places. We might refactor that...
f = open(filename, 'rb') | ||
p=Parser(inp=f, reopen_fn=reopen_fn) | ||
p.addSink(RecordIndexer()) | ||
f = open(file_name, 'rb') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using with open(...) as f
here and everywhere else we open files in this project.
On Windows, using the shebang (
#!/usr/bin/env python
) does not always work (especially with Python releases that are installed in 'portable' fashion).This branch adds a
__main__.py
to make the module executable (python -m pystdf) and moves the scripts inside the main module folder so that they can be run though the module execution.Examples:
Some of the scripts did not run, these are fixed.
Building an sdist package is still possible.
Not tested on Python 2.7 since pystdf seems to be Python 3.x only.