save method respects base path for relative paths

[UnicodingUnicorn]

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)

[roll]

@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)

[UnicodingUnicorn]

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.

[roll]

@UnicodingUnicorn
Sorry for the confusion it's my fault not properly checking the initial issue before posting it on Bountysource.

I've dove into it now. I think that the feature request #205 makes sense (I thought it was another bug).

Could you please:

• add to_base_path (bool) (better idea for the name?) flag for the behavior you have implemented. So by default, there will be no changes
• a few tests to validate new feature

[jdahlbaek]

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 to base_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.

[UnicodingUnicorn]

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.

[roll]

@jdahlbaek
It seems a perception difference 😃

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

[roll]

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

[akariv]

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.

[UnicodingUnicorn]

Yeah, I see the point on backward compatibility now. I will implement stuff as per @roll's suggestion.

[UnicodingUnicorn]

@roll may I request another review please?

[roll]

Great work. Thanks!

[lwinfree]

hi @UnicodingUnicorn! Thanks for this! Please claim the bounty 😃