Avro
  1. Avro
  2. AVRO-202

Add __all__ listing to avro module

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: python
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Folks who want to say "from avro import *" will need this listing: http://docs.python.org/tutorial/modules.html#importing-from-a-package.

      1. AVRO-202.patch
        0.6 kB
        Jeff Hammerbacher
      2. AVRO-202.patch
        3 kB
        Jeff Hammerbacher
      3. AVRO-202.patch
        2 kB
        Patrick Hunt
      4. AVRO-202.patch
        2 kB
        Jeff Hammerbacher

        Activity

        Hide
        Patrick Hunt added a comment -

        -1, please add a test for this

        I reviewed the patch and tested in an interactive shell and it worked fine, just add a test and we should be good to go.

        Show
        Patrick Hunt added a comment - -1, please add a test for this I reviewed the patch and tested in an interactive shell and it worked fine, just add a test and we should be good to go.
        Hide
        Jeff Hammerbacher added a comment -

        Thanks for the review, Patrick. I added a test, but have a few qualms about it:

        1) You can only execute "import *" at the module level; I'd rather just do this in the test.
        2) We need to maintain a list of exported modules in two places now: _init_.py and testimport.py.

        Let me know what you think.

        Thanks,
        Jeff

        Show
        Jeff Hammerbacher added a comment - Thanks for the review, Patrick. I added a test, but have a few qualms about it: 1) You can only execute "import *" at the module level; I'd rather just do this in the test. 2) We need to maintain a list of exported modules in two places now: _ init _.py and testimport.py. Let me know what you think. Thanks, Jeff
        Hide
        Patrick Hunt added a comment -

        instead you could change a test like src/test/py/testioreflect.py it's already doing import as, you could piggyback on that test (to do import *)

        neither option is great, I don't know though if it's necessary to test that all files are imported, perhaps this is a good middleground?

        Show
        Patrick Hunt added a comment - instead you could change a test like src/test/py/testioreflect.py it's already doing import as, you could piggyback on that test (to do import *) neither option is great, I don't know though if it's necessary to test that all files are imported, perhaps this is a good middleground?
        Hide
        Jeff Hammerbacher added a comment -

        Would you want a test to ensure that the appropriate modules are in the namespace, or do you just want the import * code path exercised? I don't even see a good place in testioreflect.py to test this feature.

        Sorry for being dense. I come from a world where there are no tests for code, and this piece of code is particularly hard to test.

        Show
        Jeff Hammerbacher added a comment - Would you want a test to ensure that the appropriate modules are in the namespace, or do you just want the import * code path exercised? I don't even see a good place in testioreflect.py to test this feature. Sorry for being dense. I come from a world where there are no tests for code, and this piece of code is particularly hard to test.
        Hide
        Patrick Hunt added a comment -

        fixed two issues with the patch
        1) patch failed to apply against latest trunk
        2) test failed (m should have been module_)

        Show
        Patrick Hunt added a comment - fixed two issues with the patch 1) patch failed to apply against latest trunk 2) test failed (m should have been module_)
        Hide
        Patrick Hunt added a comment -

        I'm fine with the patch the way it is currently, +1. I don't see how to resolve testing the import * w/o actually testing it fully.

        Jeff FF to submit the patch if you are ok with the way it stands currently, or change and I can review addl if you like, once
        "submitted" I'm sure someone will commit.

        Show
        Patrick Hunt added a comment - I'm fine with the patch the way it is currently, +1. I don't see how to resolve testing the import * w/o actually testing it fully. Jeff FF to submit the patch if you are ok with the way it stands currently, or change and I can review addl if you like, once "submitted" I'm sure someone will commit.
        Hide
        Philip Zeyliger added a comment -

        +1.

        Show
        Philip Zeyliger added a comment - +1.
        Hide
        Jeff Hammerbacher added a comment -

        Hey Patrick,

        I'll clean this up and mark ready to submit as soon as the Apache SVN comes back (flakiest SVN repository evar this week). Thanks for your heroic work on reviewing today.

        Regards,
        Jeff

        Show
        Jeff Hammerbacher added a comment - Hey Patrick, I'll clean this up and mark ready to submit as soon as the Apache SVN comes back (flakiest SVN repository evar this week). Thanks for your heroic work on reviewing today. Regards, Jeff
        Hide
        Jeff Hammerbacher added a comment -

        Added 'datafile' to module list in both (see AVRO-201)

        Show
        Jeff Hammerbacher added a comment - Added 'datafile' to module list in both (see AVRO-201 )
        Hide
        Doug Cutting added a comment -

        > flakiest SVN repository evar this week

        This was mostly scheduled downtime, although perhaps poorly communicated.

        For alerts, befriend http://twitter.com/infrabot

        To check server status, visit http://monitoring.apache.org/status/

        For real-time answers, go to #asfinfra on irc.freenode.net.

        Show
        Doug Cutting added a comment - > flakiest SVN repository evar this week This was mostly scheduled downtime, although perhaps poorly communicated. For alerts, befriend http://twitter.com/infrabot To check server status, visit http://monitoring.apache.org/status/ For real-time answers, go to #asfinfra on irc.freenode.net.
        Hide
        Patrick Hunt added a comment -

        Re "Added 'datafile' to module list in both (see AVRO-201)"

        I saw that but assumed that datafile is an internal detail that you typically don't want to expose through import *, no?

        Re "heroic work"

        all in a days work for a commiter - as Doug often reminds me that's the primary responsibility of a commiter.

        As I mentioned (doug correct me if I'm wrong, but this is the way ZK works), I think you'll find reviews come quicker if you "submit" the JIRA once you are satisfied with your patch. earlier reviews are fine too, but submission raises the issue on the radar since the JIRA will show up on the "patch available" list.

        Good work, nice to see you contributing changes, python WTF!

        Show
        Patrick Hunt added a comment - Re "Added 'datafile' to module list in both (see AVRO-201 )" I saw that but assumed that datafile is an internal detail that you typically don't want to expose through import *, no? Re "heroic work" all in a days work for a commiter - as Doug often reminds me that's the primary responsibility of a commiter. As I mentioned (doug correct me if I'm wrong, but this is the way ZK works), I think you'll find reviews come quicker if you "submit" the JIRA once you are satisfied with your patch. earlier reviews are fine too, but submission raises the issue on the radar since the JIRA will show up on the "patch available" list. Good work, nice to see you contributing changes, python WTF!
        Hide
        Jeff Hammerbacher added a comment -

        I saw that but assumed that datafile is an internal detail that you typically don't want to expose through import *, no?

        It's not clear to me how you would write Avro data to a file without this module. I believe it needs to be public.

        python WTF!

        FTW, I hope!

        Show
        Jeff Hammerbacher added a comment - I saw that but assumed that datafile is an internal detail that you typically don't want to expose through import *, no? It's not clear to me how you would write Avro data to a file without this module. I believe it needs to be public. python WTF! FTW, I hope!
        Hide
        Patrick Hunt added a comment -

        wow. wrong tla there "dyslexics of the world untie!"

        Show
        Patrick Hunt added a comment - wow. wrong tla there "dyslexics of the world untie!"
        Hide
        Doug Cutting added a comment -

        I just committed this. Thanks, Jeff.

        Show
        Doug Cutting added a comment - I just committed this. Thanks, Jeff.

          People

          • Assignee:
            Jeff Hammerbacher
            Reporter:
            Jeff Hammerbacher
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development