Skip to content
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

python: backward compatibility issue #180

Closed
sim590 opened this issue Mar 28, 2017 · 6 comments
Closed

python: backward compatibility issue #180

sim590 opened this issue Mar 28, 2017 · 6 comments

Comments

@sim590
Copy link
Contributor

sim590 commented Mar 28, 2017

The whole benchmark is broken by 768bfa5 since the SockAddr is not used by the benchmark and no "legacy" signature (with address as string) is still available in the python API for bootstraping 😒.

This is the case for benchmark, but also all python code which have already been written before 768bfa5. For instance, the pingpong script is broken. I think that some of ring's dht small swarm is using python bindings also. This is dangerous.

This kind of changes should be made more carefuly in the future since we never know who may use the python bindings also and we don't want to just drop support for API as fundamental as DhtRunner.bootstrap. I suggest, a legacy signature should be provided. If we want to deprecate it, then we could add a message on standard error specifying the matter, but anyhow I don't think it's good idea to remove this signature before a long time (couple of weeks or months).

@aberaud
Copy link
Member

aberaud commented Mar 28, 2017

it's a bug, this is exactly the kind of things that will be detected automatically with unit tests

@sim590
Copy link
Contributor Author

sim590 commented Mar 28, 2017

Oh. I just noticed that you in fact kept the legacy signature available. I didn't look close enough at first. So, yeah, like you say, this is a bug 😉

@aberaud
Copy link
Member

aberaud commented Apr 10, 2017

Fixed by 99f8c29

@aberaud aberaud closed this as completed Apr 10, 2017
@sim590
Copy link
Contributor Author

sim590 commented Apr 10, 2017

@aberaud: But now, the bootstrap signature bootstrap(string, string) is not present anymore which was in part what I was complaining about. Don't you think that this canonical signature should always be available?

@aberaud
Copy link
Member

aberaud commented Apr 10, 2017

I think bootstrap(string, string) was always present (as you noticed above) and still is, but was not working because of a conflict with the now-removed signature.

@sim590
Copy link
Contributor Author

sim590 commented Apr 11, 2017

@aberaud. I really have trouble reading github's diffs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants