-
Notifications
You must be signed in to change notification settings - Fork 190
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
v2 API (feeds) #1338
base: master
Are you sure you want to change the base?
v2 API (feeds) #1338
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1338 +/- ##
============================================
- Coverage 90.41% 90.16% -0.25%
- Complexity 761 765 +4
============================================
Files 66 67 +1
Lines 2806 2807 +1
============================================
- Hits 2537 2531 -6
- Misses 269 276 +7
Continue to review full report at Codecov.
|
*/ | ||
public function create( | ||
string $url, | ||
string $name = '', |
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.
I think this should be like this
string $name = '', | |
?string $name = null, |
string $basicAuthUser = null, | ||
string $basicAuthPassword = null |
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.
These will fail with the default because null
is not a string
string $basicAuthUser = null, | |
string $basicAuthPassword = null | |
?string $basicAuthUser = null, | |
?string $basicAuthPassword = null |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
The API documentation lists several error return messages with different status codes but as far as I can see there is no possibility to distinguish between those in the services. Should I try to add this to the service or change/remove it from the docu since it's not possible at the moment? |
There are a bunch of errors like "folder name conflict" that don't make sense to error on and have been removed AFAIK. I'd just update the doc for now. |
Signed-off-by: Paul Tirk <paultirk@paultirk.com>
Ok, I'll adapt the API documentation accordingly. We can always add more error codes later if it's possible or desired. |
Author of RSS Guard here. No bad intentions intended: I just want to ask, in what state is APIv2 right now? Is it far from being complete/usable? Any ETA? Why I ask as that I have 1.2 API implemented in RSS Guard (and used by many people) and I want to plan some related things. Thanks. |
Only folders are implemented, the rest is |
I was planning to implement the v2 API but I have very little time (sorry 🙁) and I got stuck because in my opinion the API is not really finished. For example, there is no discussion about what should happen if there are thousands of items on initial sync.. I think an API should really be solid and I don't have much experience with those kind of things so I don't know how to proceed.. |
As I have quite extensive experience with CONSUMING all kinds of APIs, including "RSS" ones, all I can say is that if you do not want to reinvent the wheel, simply implement Google Reader API. It is relatively sane API and if implemented well by both server and client, works fast too + it is already implemented in plethora of clients. https://github.com/bazqux/bazqux-api That said, I really do not think that 1.2 API is bad. It is actually quite good! It only misses some options to be "perfect":
|
Don't be sorry, any effort is much appreciated.
Unfortunately that's near impossible within the constraints of being a nextcloud app.
There is only one ID, it's the nextcloud ID, any other information is purely for convenience.
Would you mind elaborating on that a bit? This seems like something that might be possible to add without going to a V2 directly. |
Let's imagine user only wants to efficiently download unread only articles from some feed and feed contains big number of changing unread articles for some reason (trust me, I've seen use-cases you would not imagine in your worst dreams). He calls "Get items" with "getRead=false" and when he wants to check what new unread items there are later, he must call "Get Items" with "getRead=false" again, because method "Get updated items" does not allow to filter with "getRead" parameter (at least docs do not say it can). Here is where my proposition comes into play. Design new method "Get items IDs", which will take same input parameters like "Get items" but will be fast and will only produce list of item IDs. Then have one more extra method "Get items from IDs" which will work exactly like "Get items" but will not return items from folder/feed, but from provided IDs list. That ways, client can call "Get items IDs" to get various "sets" of IDs (list of read IDs, list of unread IDs, list of starred IDs), then compare with local app DB what articles he does not have, what articles moved read=>unread, unread=>read and then download full articles with only IDs he really really wants. This minimizes network bandwith dramatically, particularly for BIG feeds. I agree that "Get updated items" covers some use-cases, just not all of them. I recommend reading part of that Bazqux API: https://github.com/bazqux/bazqux-api#the-right-way-to-sync So current API only really needs to polish and extend a bit, just a couple of parameters, couple of methods. As I see it, we do not really need whole new API as there are really not "design" problems (apart from that doubled article ID thing, which only concerns one method). Just my opinion, cheers. |
Martin's suggestions seem like a good idea to me, since I'm not completely convinced of the "finished" state of the v2 API. Would it be acceptable to address those two points (maybe make it an API 1.3 or so) @SMillerDev ? I might look into that for the moment.. |
Yeah, I'm fine with improving the current API |
Feeds part of the v2 API