Skip to content

fire.py: Use fire on arbitrary file or module. #35

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

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

nealmcb
Copy link
Contributor

@nealmcb nealmcb commented Mar 26, 2017

Based on the discussion at Use fire without editing code directly · Issue #29 · google/python-fire

Of course, still needs better error handling, documentation, installation support, etc.

Based on the discussion at [Use fire without editing code directly · Issue google#29 · google/python-fire](google#29)

Of course, still needs better error handling, documentation, installation support, etc.
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@nealmcb
Copy link
Contributor Author

nealmcb commented Mar 26, 2017

It seems to pass the Travis CI on all the python 3 versions tested, but not python 2. But in my own testing it works fine on python 2, and I haven't figured out where it's going wrong in fire.fire_import_test.FireImportTest with

AttributeError: 'module' object has no attribute 'Fire'

Hints welcome :)

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@dbieber
Copy link
Collaborator

dbieber commented Mar 27, 2017

Thanks for starting this.

Travis
The immediate problem is that you've added a file named fire.py, so the test that tries to "import fire" ends up importing your file as the fire module, rather than the actual fire module. Using a different file name will get around this issue.

Development workflow
Like you say, there's other work to do before merging this to master (tests, documentation, etc). I'd be open to creating a feature branch for this if you prefer to work on this over multiple separate pull requests.

Code review
From a quick glance at the script, one issue I notice is that we shouldn't be modifying sys.argv. Instead, we'll want to modify the Fire function to accept a list for argv (currently it only accepts a string for the argument 'command', not a list).
I'll also try to get a pylintrc into the repo / checked by travis to make staying stylistically consistent easier.

@nealmcb
Copy link
Contributor Author

nealmcb commented Mar 27, 2017

Thanks, @dberlin . That was my hunch for the import issue, but it makes me yet more curious about why it works in python 3.
A feature branch sounds helpful.

@dbieber dbieber changed the base branch from master to issue29-fire-without-edits March 27, 2017 20:07
@dbieber
Copy link
Collaborator

dbieber commented Mar 27, 2017

I changed the base to 'issue29-fire-without-edits'.

@jtratner
Copy link
Contributor

Another thing that's missing here is the ability to resolve to classes or functions within a module (tho I guess that's really just quibbling over addition or removal of an extra space)

@dbieber
Copy link
Collaborator

dbieber commented Mar 28, 2017

Classes or functions within a module (tho I guess that's really just quibbling over addition or removal of an extra space).

If they're within a module (e.g. PIL.Image) then it's just a matter of using a space instead of a dot.

If we want to support builtin classes/functions (e.g. raw_input, type, etc), then we need more than just find_module.

nealmcb added 2 commits March 28, 2017 12:57
Should fix the build problem for python 2.  Later, get it installed as "fire" via setup.py
Following advice from Python Apps the Right Way: entry points and scripts | Chris Warrick
 https://chriswarrick.com/blog/2014/09/15/python-apps-the-right-way-entry_points-and-scripts/
@jack17529
Copy link

@dbieber is the PR done or something still remains?

@dbieber
Copy link
Collaborator

dbieber commented Apr 5, 2017

I am OK w/ merging this to the issue29-fire-without-edits branch. Apologies for not merging sooner.

Some things that remain before merging to master:

  • Integration into setup.py
  • Documentation
  • Removing requirement of -f/-m flag.
  • lint compliance

@dbieber dbieber merged commit 7154510 into google:issue29-fire-without-edits Apr 5, 2017
@nealmcb
Copy link
Contributor Author

nealmcb commented Apr 5, 2017

Thanks.

Also needed: tests! Input on good testing approaches and frameworks for command-line functionality
would be welcomed.

@jack17529
Copy link

@nealmcb I would love to work on tests as I have recently studied them, might require some help , can I take this up?

@nealmcb nealmcb deleted the patch-1 branch April 7, 2017 22:06
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.

5 participants