Fixes issue (127) where append_date_dirs wasn't working properly. #130
Conversation
| try: | ||
| target_path = t.cast(str, params['target_path']) | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
A more conventional method we use in our codebase is to call get with a default value. Would something like:
target_path = t.cast(str, params.get('target_path', ''))
work here?
There was a problem hiding this comment.
Yep that should be fine. Updating the PR
pbattaglia
force-pushed
the
append_date_dirs_127
branch
from
48bf90e
to
27d63eb
Compare
* Fixed bug where partition_keys was being modified in-place, and causing problems laters. * Adjusted how target_path is constructed so that trailing forward slashes in target_path are all removed. * Added typing.cast to comply with pytype.
pbattaglia
force-pushed
the
append_date_dirs_127
branch
from
27d63eb
to
dace796
Compare
| if str(params.get('target_path')).endswith('/'): | ||
| params['target_path'] = params.get('target_path')[:-1] | ||
| target_path = t.cast(str, params.get('target_path', '')) | ||
| if target_path != '': |
There was a problem hiding this comment.
One last nit: It this if statement necessary? I think rstrip on an empty string will produce an empty string.
There was a problem hiding this comment.
Right, but I added it because you don't want to add a key to the dict if it doesn't already have it.
There was a problem hiding this comment.
Though, generally, I'm not actually sure why this is located here, because it seems like it'd be more natural to handle it in prepare_target_name().
On a similar note, I think subtracting 1 from num_partition_keys here, but actually removing the "date" partition key in prepare_target_name(), might be a little risky. Could they be done in the same place?
There was a problem hiding this comment.
Right, but I added it because you don't want to add a key to the dict if it doesn't already have it.
Ah, that makes sense.
it seems like it'd be more natural to handle it in prepare_target_name().
I agree – this would be a better fit: It'd be best if the parser had less logic, in general.
Could they be done in the same place?
Yes, that totally sounds right to me.
Last: I think we are no longer using the num_partition_keys counter anymore. It would be prudent to pull that out.
Fixed bug where partition_keys was being modified in-place, and causing problems laters.
Adjusted how target_path is constructed so that trailing forward slashes in target_path are all removed.
Added typing.cast to comply with pytype. I'm not sure why this wasn't triggered previously, but I was getting pytype errors from the prepare_target_name() function, which I was editing, so I fixed it in this PR.