Arvados

Looking at b4a9aaa

This looks like it won't play well with "attr asc" and "attr desc" forms (and seems not to have any tests): update, fixed in de54cdc

@orders.select! { |o| @select.include? o } if @select

Add a test with more "select" than "distinct" columns (or no "select" at all), and verify that the "distinct" behavior actually occurs

Mention at doc/api/methods.html

Otherwise, LGTM

In docs: prefer "false (default)" rather than "default; false:" -- and/or mention the false case first. Currently it's easy to read as if true is the default.

Reviewing e6aa5b4. I love a well-tested branch. Thanks for building all this necessary infrastructure; I'm sure I'll be building on top of it before I know it. My comments are hardly about the code itself; more about glue.

The one thing I'd say about the updated driver is that there seems to be a lot of similar code, especially in the event bus subscriptions and invalidate methods. The merge method does a great job of abstracting away common functionality—I wonder if it'd be possible to use a similar strategy to refactor these too? But this is more along the lines of style and I don't think it needs to hold up the merge before the end of the sprint.

The setup.py templates assume that we launch it from its own directory when they rmtree. Because this is potentially destructive, I think it's worth guarding to make sure we work on the correct build directory. It also causes the script to fail if we haven't built anything yet, or didn't last time. Maybe something like:

import os
import sys
build_dir = os.path.join(os.path.dirname(sys.argv[0]), 'build')
shutil.rmtree(build_dir, ignore_errors=True)

And then a couple small things in run_test_server.py:find_server_pid:

• Testing server_pid > 0 is extraneous. It's in the Ruby because ''.to_i == 0 (i.e., the pidfile is empty), but in Python int('') is a ValueError. I would expect the outcomes here to always be IOError, OSError, ValueError, or a good pid.
• Naming those exceptions, or at least catching Exception, would be helpful to avoid catching low-level exceptions like KeyboardInterrupt (since it's conceivable the user might try to ^C during the loop sleep).

Thanks.

One small additional note: setup_fuse.py should be added to the Python SDK .gitignore alongside setup.py.

The event bus and invalidation code is more or less just the minimum required to ensure that API server updates are visible to the user. A better implementation would actually look at the event to determine what to do with it.

I applied your suggestion for selecting the correct directory, and that inspired me to refactor the setup scripts to eliminate build.sh and sed.

I applied the changes you suggested to run_test_server.py

The .gitignore comment is no longer applicable because setup.py and setup_fuse.py are no longer generated from setup.py.src and setup_fuse.py.src.

Peter Amstutz wrote:

I applied your suggestion for selecting the correct directory, and that inspired me to refactor the setup scripts to eliminate build.sh and sed.

That's even way better, thank you. This version looks good to merge to me.