Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

florisla
Copy link
Contributor

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:

python -m pystdf txt mydata.stdf
python -m pystdf xlsx mydata.stdf
python -m pystdf xml mydata.stdf
python -m pystdf count mydata.stdf
python -m pystdf slice mydata.stdf 10 55

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.

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.
Copy link
Owner

@cmars cmars left a 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.

Copy link
Owner

@cmars cmars left a 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']:
Copy link
Owner

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.

Comment on lines +38 to +39
GZ_PATTERN = re.compile('\.g?z', re.I)
BZ2_PATTERN = re.compile('\.bz2', re.I)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Owner

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')
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants