#1685 Add support for multiple results in salesforce batch jobs #1686
Conversation
|
By analyzing the blame information on this pull request, we identified @dlstadther to be a potential reviewer |
|
Based on TravisCI results, looks like you just need to fix the few flake8 errors and then run the tests again. All the others look to be errors when creating the environment (stuff you didn't cause). I haven't run this myself, but the logic looks good. Thanks! |
|
D'oh. Of course I forget to run the flake8 test after I add the unit test. ;) Addressing shortly. |
dacort
changed the title
|
Looks like you need to get @Tarrasch 's updates to Tox (that'll fix these failing tests). Also, did you fix the duplicated header issue? |
Adds a new method 'get_batch_result_ids' and maintains backwards compatibility with the old method, adding a warning.
Note that this only supports CSV since this is what the library does by default.
|
@dlstadther Thanks for the note re: Tox updates. Just pulled that in and pushed up an update that merges batch results (from CSV). |
|
Looks like I have to some work to do re: python 3 compat. |
| if sys.version_info.major > 2: | ||
| patch_name = 'builtins.open' | ||
|
|
||
| @mock.patch(patch_name, side_effect=mocked_open) |
There was a problem hiding this comment.
Is it possible to use six.py for the Python 2.x vs 3.x compatibility?
There was a problem hiding this comment.
Not sure if I can use it to import a generic builtins, but I can at least use it to check for the Python version.
The challenge here is the patch_name string gets interpreted further down the stack and I'm not actually importing builtins, so flake8 throws an error.
There was a problem hiding this comment.
Well, tests passed. So if @Tarrasch approves, then i think this pr is good to go.
Current coverage is 76.10%@@ master #1686 diff @@
==========================================
Files 94 94
Lines 10256 10285 +29
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 8063 7827 -236
- Misses 2193 2458 +265
Partials 0 0
|
| @@ -135,6 +135,10 @@ def rename(self, *args, **kwargs): | |||
| def path(self): | |||
| return self._fn | |||
|
|
|||
| @property | |||
| def fn(self): | |||
There was a problem hiding this comment.
Instead of using .fn can't you just use .path and there's less code in total?
There was a problem hiding this comment.
Looks like I can, yep! Had just been following the convention that was already in the module.
Switched over to using .path - that works fine. Also removed extra code that had been added in mock.py.
There was a problem hiding this comment.
My bad for setting a poor example within salesforce.py!
dacort
changed the title
|
Good job. Thanks guys :) |
Addresses #1685 by adding support to fetch multiple results from a single Salesforce batch job.
Open Items
Description
get_batch_result_idsmethod that is more accurately documentedget_batch_resultsmethod, including warning messageMotivation and Context
Fixes #1685
Have you tested this? If so, how?
Included unit tests as well as verified locally.