1. Mike Miller
  2. pytave
  3. Pull requests

Pull requests

#13 Merged
Repository
Deleted repository
Branch
default (0500459a739b)
Repository
pytave
Branch
default

Enforce checking of attributes in builtins for pycall (fixes issue #20)

Author
  1. Abhinav Tripathi
Reviewers
Description
  • pycall.cc: Add the checks to search for an attribute in main and if not found then revert to builtin or builtins whichever is available. Also add a test to demonstrate this
  • Issues #20: Make calling builtin functions easier resolved

Comments (9)

  1. Abhinav Tripathi author

    Although this works, it might be too naive approach to follow.
    I put it here just for suggestions.
    I see 2 options here.
    1 - Call pyexec from pycall to do the conditional checking for either __builtin__ or builtins depending on python version.
    That still leaves us to check if the attribute is present in __main__ or in builtins namespace.
    2 - Use boost_python 's c++ api somehow to do the above check

  2. Colin Macdonald

    I haven't read over this carefully but I think I would like to see an explicit python version check.

    I feel like we should minimize our boost usage if anything, IIRC, there was a vague plan to get rid of the boost dependency.

  3. Colin Macdonald

    Some minor comments below but after that it LGTM!

    For a future change---not needed for htis PR---maybe a run-time check rather than compile time macro check is more flexible. Some day it would be good to support dynamically selecting python at runtime.

  4. Abhinav Tripathi author

    Colin Macdonald I updated the PR with the proper edits. The line mod = import ("builtins"); actually has proper spacing. (I checked it has 2 4 spaces from if and 2 from {. .

    maybe a run-time check rather than compile time macro check is more flexible

    Sure. I'll try to see how to do that. Probably using sys.versioninfo but will have to check how to call that and get the response.

  5. Mike Miller repo owner

    I've merged this. The logic here starts to look a little cluttered, so I pushed a follow up commit to try to flatten it a bit by separating the builtins logic from the rest of it, see 9077907.

    Also your check for whether module has the function was wrong so I took that out. That would have meant that something like pycall("math.str") would fail to find the function str in math and fall back to looking for it in the local or global namespaces.

    And you had mixed tabs and spaces, that is now fixed. Please try to use spaces only going forward.

    I am not actually sure that the Python 2 vs 3 check could be made a runtime check at this level. The libpython API is different, so compiling it against one or the other results in something that can't work with both versions. I think if we want to support both Python 2 and 3, maybe we will need separate .oct files under a facade interface, something I've thought about, but way in the future.

  6. Abhinav Tripathi author

    The logic here starts to look a little cluttered, so I pushed a follow up commit to try to flatten it a bit by separating the builtins logic from the rest of it

    Thanks. I just saw the follow up commit. Definitely better.
    .

    Also your check for whether module has the function was wrong so I took that out. That would have meant that something like pycall("math.str") would fail

    Oh. Thanks again. I didn't think of such a case.
    .

    And you had mixed tabs and spaces, that is now fixed. Please try to use spaces only going forward.

    Sorry. I used gedit and made sure to replace all tabs with spaces before committing. I might have forgot at a few places.
    .

    I think if we want to support both Python 2 and 3, maybe we will need separate .oct files under a facade interface, something I've thought about, but way in the future.

    Sure. Maybe sometime after everything is in place and working properly then we can look into having 2 oct files.