Add service newsblur - #183
Conversation
Creating a basic ServiceHooks object for the NewsBlur API
remove console.log
half implementation of missing hooks
Update last changes from Fluentflame
|
Oh, and I was signing with the wrong key. My bad |
|
I haven't forgotten about this, but there's a lot to go over here (and indeed, we should squash much of this). BUT I appreciate the effort, and I'll go through everything detailed soon-ish. |
CrystalSplitter
left a comment
CrystalSplitter
left a comment
There was a problem hiding this comment.
This is good work! I think we can work with this, actually.
There's a bunch of style issues, but I do not consider these a blocker and I can clean them up myself, as it's no hassle for me.
I've left minor comments, but overall my only question here is the large amount of commented out tests and todos.
I'm not quite ready to approve, but I think we're close.
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
We shouldn't add this to the main git repo. Also, do make sure you format all files you do touch!
| global.console.error = mocks.consoleError; | ||
| }); | ||
|
|
||
| /* |
There was a problem hiding this comment.
What's this for? Did you mean to leave this in?
|
|
||
| const feeds: Record<string, NewsblurFeed> | undefined = response.feeds; | ||
|
|
||
| if (feeds === undefined) { |
There was a problem hiding this comment.
Prefer if (feeds == null) as I imagine if response.feeds is in fact null, our response is the same. Update the error message below accordingly.
| throw APIError("property 'feeds' is undefined"); | ||
| } | ||
|
|
||
| const sources: RSSSource[] = []; |
There was a problem hiding this comment.
Prefer functional style:
const sources = Object.entries(feeds).map(([_k, v]) => new RSSSource(v.feed_address, v.feed_title));| // minTime to avoid overwhelming the server | ||
| configs._lastRefresh = new Date(); | ||
| // correct? | ||
| return json.authenticated === true; |
There was a problem hiding this comment.
Why not return json.authenticated?
| // correct? | ||
| return json.authenticated === true; | ||
| } catch { | ||
| return false; |
There was a problem hiding this comment.
Log the error somewhere so it's not silently omitted?
| const url = new URL(configs.endpoint + path); | ||
| // set headers | ||
| const headers = new Headers(); | ||
| headers.set("Content-Type", "application/x-www-form-urlencoded"); |
There was a problem hiding this comment.
Huh, interesting. This is fine, but why?
| params: ParamsObject, | ||
| ) { | ||
| // set url | ||
| while (path.startsWith("/")) path = path.substring(1); // remove leading slash |
There was a problem hiding this comment.
nit: Why? We control the path for all callers.
| @@ -0,0 +1,422 @@ | |||
| import { ServiceConfigs, SyncService } from "../../../schema-types"; | |||
There was a problem hiding this comment.
Can you put a comment at the top of this indicating that this is still a work in progress?
|
Also can you squash/rebase your commits into three commits:
Use
and then mark the commits as |
|
Wow. Thank you so much for taking the time to review it. I sort of lost my "nord" coding this, but now I really know what to do. I'll take the time to answer each question... eventually (I think I can do it on my phone with the GitHub app, so Also, I didn't know the commits could be rebased, I thought once it was committed, you couldn't change it anymore. I'll make a more proper pull request next time. Once again, thank you! |
2 participants
Hello, this is my pull request to add the service Newsblur to the Fluentflame project.
I have added the files
newsblur.tsandtest_newsblur.ts.For the implementation, I have copied other implementations from the folder
services/and adapted them to Newsblur.For the development, I used:
pnpmas package managerThere is some stuff that I still don't understand about Redux and Fluentflame Reader, like when to use dispatch and what should it return, or where are RSSSources and RSSItem stored.
However, I am pretty shure that the
authenticate(),updateSources(),syncItems()andmarkAllRead()are implemented correctly.I stay available for changes, concerns, etc. I'd really like to learn how it's implemented in its entirety.