-
Notifications
You must be signed in to change notification settings - Fork 111
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
RF: Implement new API (add) #612
Conversation
TST: Skip test_add for now to see everything else works Add notes in preparation of implementing
Codecov Report
@@ Coverage Diff @@
## master #612 +/- ##
==========================================
- Coverage 86.79% 86.76% -0.03%
==========================================
Files 198 200 +2
Lines 17903 18124 +221
==========================================
+ Hits 15539 15726 +187
- Misses 2364 2398 +34
Continue to review full report at Codecov.
|
Note: this command should query a datasets configuration to decide how to add files. For example, I might want to configure that any files in |
return return_values | ||
|
||
@staticmethod | ||
def result_renderer_cmdline(res): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API has changed: see #740
Observation:
|
Recursive does not work as expected (continued scenario from above):
I guess it doesn't realize that |
Otherwise this is looking good, I think. I'd appreciate quick fixes and a swift merge (that I'd myself ;-) |
See bpoldrack#30 for some trivial fixes. |
BF: Trivial fixes
Regarding recursion: Currently unclear to me, whether this is what we want. Need to talk, I guess. Note, that |
Regarding your observation of reporting success twice in case of |
FTR: IMHO: any argument needs to be resolved against the root of the respective dataset (that had its |
Agree. That's exactly, what I'm currently implementing. |
Tests fail, but otherwise looks good. |
Strange. Apparently, Travis ran on a master, that I didn't merge yet, while the buildbots used an older one and passed. |
OK, let's merge this now and unify the behavior re save in one go, once the required utilities are in. |
FYI merge caused major havoc conflict in #746 which I hopefully resolve correctly now |
actually I am wrong -- this one alone couldn't do it because of only a tiny change (force arg)... heh -- smth else then |
No description provided.