-
Notifications
You must be signed in to change notification settings - Fork 199
Rename from API to Http-Client #144
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
Conversation
Thanks for working on this again! |
@daviddias @Alexander255 |
Looks mostly good. Please take a look at those CI failures (aside from codestyle). Also please don't create the |
|
Then please delete the original files ( Thanks for taking the time to work on this and all the work you've already submitted thus far! |
522e2e3
to
88e01e4
Compare
@Alexander255 I have renamed the 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks very good! 👍
To fix the merge conflict try (where origin is the name of ipfs/py-ipfs-api
repo):
git fetch origin master
git rebase origin/master
git mergetool
In general api_ref.md
refers to the API of this module (ie: the interface our users use), but if it's easier to keep it renamed to http_client_ref.md
, it won't do any harm either 🙂
One important thing I found during review, however, is that you change the default API endpoint path from api/v0
to http_client/v0
– this is unacceptable as it would make us incompatible with all existing API server implementations for no good reason (it's one of those actually refers to API cases). Other than that I only added some comments regard your comments on URLs and names that will have to be fixed by other (ie: me) – nevermind them.
If you need help getting rid of the merge conflict just leave a comment. Once the conflict is resolved please also try to make the CI happy (except for codestyle
never mind that for now).
Ok. I'll do the required changes. Just give me a few days time. I am currently not at home and don't have my laptop. :) |
88e01e4
to
1bbd5e2
Compare
Finally! No conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI found two more issues (mentioned above) that easy to fix. I'm not sure what the the issue is but I think it may go away when I apply one of my planned (read: backward-incompatible) fixes before the release.
Thank you for the work you submitted! It's been quite some help!
with HTTMock(api_okay): | ||
res = self.client.request('/apiokay') | ||
with HTTMock(http_client_okay): | ||
res = self.client.request('/http_client_fail') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be /http_client_okay
with HTTMock(api_okay): | ||
res = self.client.request('/apiokay', | ||
with HTTMock(http_client_okay): | ||
res = self.client.request('/http_client_fail', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
I'll keep on updating
renaming_status.txt
.The file will be deleted before the final merge.
(The module may be unusable while the process of renaming is going on.)