Closed UnicodingUnicorn closed 4 years ago
@UnicodingUnicorn Thanks!
Now I'm not sure that I fully understand the initial issue. As a client, I would probably be surprised if it's changed the patch under the hood e.g.
package.save('to/my/path.json`) # I think I expect it to be in the exact path
And not sure how it will play with remote paths etc.
Could you please provide an example/test of new behaviour? And probably we need to add an option to make this change non-breaking (or my understanding is wrong)
Yeah, the initial issue is a bit weird. But you added the labels to it so I kinda assumed it was good to go. It looked simple enough, and the fact there's a bounty on it is a plus.
Looking back now, this is more a documentation issue. Maybe it's simpler to revise the description of target
. I think I'll close this pull request.
Since I authored the weird issue #205, let me try to explain.
What I should have written is probably
if
target
contains a relative path, I would expect it to be appended tobase_path
I think the docs are quite clear, stating
base_path (str): base path for all relative paths
As such, I would find it very surprising if the call
package.save('to/my/path.json`)
did not respect base_path
.
It is my understanding that the current behaviour is either a bug, or a case of misleading documentation for the target
argument.
@UnicodingUnicorn It is my understanding that your patch fixes the bug.
To ensure backwards compatibility, one could introduce an ignore_base_path
argument with default value None
. In case the user passes a relative path, emit a warning if ignore_base_path is None
, warning the user that the current default behaviour (ignore_base_path=True
) will be changed in the future (to ignore_base_path=False
), and instruct the user to set ignore_base_path
explicitly to avoid compatibility problems.
Sorry for the imprecise statement in the original issue, I hope it is more clear now what I meant.
I think the issue here is that the docs describe or imply a behaviour that is not actually implemented, so adding this functionality as a new feature as @roll suggests does not help much.
I believe that either the docs need to be updated, or the feature implemented, which admittedly is a breaking change. Although I'm a bit wary of adding new arguments, @jdahlbaek's method seems to be a good way to perform the transition.
I'm working on the tests now.
@jdahlbaek It seems a perception difference :smiley:
If I see the code like this:
package = Package(descriptor, base_path='input/datapackage.json')
...
package.save('output/datapackage.json')
I will never think about input/ouput/datapackage.json
as a target because I'm used to thinking that all relative paths are relative to the CWD
(current working directory). Out tests are written like this etc
So probably there is no a right answer for it.
But I still think that something like this:
package = Package(descriptor, base_path='input/datapackage.json')
...
package.save('output/datapackage.json', to_base_path=True)
is the least damaging considering the current state of art.
cc @akariv
I mean even if we consider that current behavior is a bug (which I don't think is correct) there can be some code relying on this behavior and we can't introduce breaking changes
I can imagine use cases in which each api seems more natural. However, from a backward compatibility point of view @roll 's suggestions makes sense to me.
Yeah, I see the point on backward compatibility now. I will implement stuff as per @roll's suggestion.
@roll may I request another review please?
hi @UnicodingUnicorn! Thanks for this! Please claim the bounty 😃
Should resolve #205, at least partially. From the wording of the issue, I cannot tell if it's supposed to apply to everything. I suppose so, but it's best to play it safe.
Please preserve this line to notify @roll (lead of this repository)