Fixed Edge all() method so id list is optional

[dusktreader]

The all() method used to require a list of ids to match.

If thelist was empty, the all() method would fail with an error: 'NoneType is not Iterable'

I made the ids list default to an empty list and fixed the method so that it would not fail when supplied an empty list of ids

[leifurhauks]

Thanks for this! There is something we'll need you to tweak; mutable default arguments (like a list) in python lead to some very subtle issues, like the following:

def append_three(li=[]):
    li.append(3)
    return li

print(append_three)
print(append_three)
print(append_three)

This will print

[3]
[3, 3]
[3, 3, 3]

This is because the default arguments are evaluated once, when the function is defined. Thus, every time the append_three function is called without the optional argument, it receives the same list instance, which was modified during the last call.

While in this case the ids argument to all isn't being modified, it's usually best to avoid mutable default arguments because they can lead to subtle bugs. Instead, use a default of None, and check for None in the function body.

[davebshow]

+1 @leifurhauks the default value for the ids kwarg should be None. Furthermore, I think the internals of this method need to be reviewed, as there are some leftovers for the previous implementation that are no longer applicable.

[davebshow]

Vertex.all needs similar review and fixes

[dusktreader]

I just updated the pull request to make the default value for ids None

[leifurhauks]

Instead of checking for the truthiness of ids throughout the function, just put one explicit check for None at the top of the function. i.e., to fix the append_three function in my earlier example, you would want to do this:

def append_three(li=None):
    if li is None:
        li = []

    li.append(3)
    return li

[leifurhauks]

Also, please update edge_io_tests.TestEdgeIO.test_all_method_for_known_bad_input.

It is failing because it expects a TypeError to be raised when all is called without the ids argument. Remove that assertion, since it's no longer applicable.

Then add a new test function (it might be called something like test_all_method_without_id_param) that tests that the all method, when called without passing the ids argument, retrieves all edges with the given label without raising an exception.

You may want to refer to the test_all_method_for_known_ids test for an example of how to structure the new test.

[dusktreader]

I tried to run tests using python setup.py test and it didn't work. Is there a preferred method of running the tests?

[davebshow]

I just added a tox.ini file so now you can simply run the tests:

$ tox

Tests do read and write from database, so use a dummy database. Also, be sure to clean before testing using:

./bin/titan.sh clean

[dusktreader]

I'm working on getting the tests going. How can I run two titan servers on the same machine. I tried unzipping a new titan directory and editing the conf/gremlin-server/gremlin-server.conf file and then running the server, but it conflicts with the instance of cassandra that's already running

[davebshow]

You can just run the single server:

$ ./bin/titan.sh start

Then from a separate terminal (will all of the goblin deps installed of course):

$ cd goblin
$ tox

Let me know if that works.

[dusktreader]

Oh, sure, I am working on it with a single server. However, I would like to get two going so that I can have a test server as well

[dusktreader]

I've fixed up the commit to address some broken tests. I needed to fix some functionality as well because some tests were breaking in other ways. Let me know what you think of the changes and if I need to adjust them further, I can.

[coveralls]

Coverage decreased (-0.1%) to 72.633% when pulling c017e9c72d799cb2bd3cc41ed3979e7bd7e697a1 on dusktreader:dusktreader/fix_all_method_of_edge_class into ab22ba1a9c59f4d1a7a831bf6fca441fb2696f07 on ZEROFAIL:dev.

[davebshow]

Thanks for all of your hard work on this @dusktreader. It seems like you have the tests up and running and you are ready to go for future PRs. +1 to that.

I agree, that elements should be filtered by label when calling all as you suggested; however, I think that if you pass illegal ids to the method you should hear about it by raising an error.

As I was saying yesterday, these methods needed more than a signature makeover, they needed an internal refactor. Furthermore, with the new stack, they need to be the same for both Vertices and Edges. These methods are also quite important, and changing them has repercussions throughout the library.

Due to this fact, I did a refactor this morning, moving the methods to the base element class and updating tests, as well as some of the internal vertex accessing patterns, which need to reflect that fact that all now filters by label. You can find the refactor in commits aeedb4f478ba064f383eec13f110a0f52f62a029 and 4cff98c8f639cb277bff4dadb5cd0408c0e1bac3.

If you have any questions or concerns let me know.

[dusktreader]

@davebshow thanks for this!