Initial inoreader implementation #22

bartlibert · 2017-10-19T14:11:11Z

Pull request to get feedback / help on this.

Currently using API keys under my account, if necessary this can be changed.

coveralls · 2017-10-19T15:39:04Z

Coverage decreased (-0.6%) to 35.74% when pulling 2e0de94ae6cc22be136f649c739f69b8aa4a66cc on bartlibert:master into f40d5eb on newsboat:master.

coveralls · 2017-10-19T15:39:04Z

Coverage decreased (-0.6%) to 35.74% when pulling 2e0de94ae6cc22be136f649c739f69b8aa4a66cc on bartlibert:master into f40d5eb on newsboat:master.

Minoru

Thanks a lot! I'm still not sure how comfortable you are with C++, so if any of my comments seem overwhelming, just say so.

I'm generally in favour of this PR, but I don't like the hard-coded tokens. Will you be interested in porting this to OAuth? Newsboat don't have any existing code for such APIs, though, and I don't have any experience with OAuth, but I'm ready to discuss things with you and help with tying this into Newsboat nicely.

Another thing: if someone reports a problem with Newsboat and Inoreader, can I ping you to take a look? I'm asking because I'm not using Inoreader and I won't notice if something breaks; and since I don't use it, I might not fully comprehend user's reports. In other words: will you maintain this particular class, handling bugs reported against it and aiding in implementing features?

Speaking of users: please write some docs! You'll probably be able to base them off The Old Reader's doc entry.

I didn't test the code yet, but I will—on the weekend, hopefully.

Minoru · 2017-10-19T17:48:24Z

include/inoreader_api.h

@@ -0,0 +1,48 @@
+#ifndef NEWSBEUTER_INOREADER_API_H


I recently reworked all headers to use another format, so this should say "NEWSBOAT_INOREADER_API_H_".

Will do. I started this out on Newsbeuter still at the time, so that's the reason probably :)

Minoru · 2017-10-19T17:51:35Z

mk/mk.deps

@@ -31,7 +31,7 @@ src/cache.o: src/cache.cpp include/controller.h include/urlreader.h \
 include/matcher.h filter/FilterParser.h include/utils.h include/logger.h \
 config.h include/strprintf.h include/cache.h include/filtercontainer.h \
 include/colormanager.h include/regexmanager.h include/remote_api.h \
- include/exceptions.h
+ include/fslock.h include/exceptions.h


Oh, that's my bad! Forgot to run make depslist after adding FSLock.

If you are comfortable with git rebase, I'd rather see this little change as a separate commit—i.e. check out master, run make depslist, commit, rebase your PR on top of the resulting commit.

If rebase isn't something you can do easily, don't bother; it's just a tiny detail, nothing show-stopping.

No problem, I'll rebase this.

Minoru · 2017-10-19T17:54:07Z

src/configcontainer.cpp

@@ -65,6 +65,9 @@ configcontainer::configcontainer()
 		{ "ignore-mode",
 		    configdata("download", std::unordered_set<std::string>({
 		        "download", "display" })) },
+		{ "inoreader-login", configdata("", configdata_t::STR) },
+		{ "inoreader-password", configdata("", configdata_t::STR) },


All our remote APIs support -passwordfile and -passwordeval, so Inoreader should, too.

Minoru · 2017-10-19T17:56:34Z

src/inoreader_api.cpp

+
+std::string inoreader_api::retrieve_auth() {
+	CURL * handle = curl_easy_init();
+	credentials cred = get_credentials("inoreader", "inoreader");


Other APIs use capitals in second argument here, because it's the name of the service; please change it to "Inoreader".

Minoru · 2017-10-19T18:03:31Z

src/inoreader_api.cpp

+	curl_slist_free_all(list);
+
+	std::vector<std::string> lines = utils::tokenize(result);
+	for (auto line : lines) {


You aren't modifying those strings, so make it const auto. Then, it also doesn't make sense to ask for a copy, so change it to const auto& to use references. (Just as with ++i vs. i++ in for loops, it doesn't matter in this particular case because items are few and quite short, but you better cultivate the right habit for the times when this stuff does matter.)

Minoru · 2017-10-19T18:04:17Z

src/inoreader_api.cpp

+	std::vector<std::string> lines = utils::tokenize(result);
+	for (auto line : lines) {
+		LOG(level::DEBUG, "inoreader_api::retrieve_auth: line = %s", line);
+		if (line.substr(0,5)=="Auth=") {


Can you add spaces around ==? Easier to read this way, especially since the string ends with = too.

Minoru · 2017-10-19T18:04:48Z

src/inoreader_api.cpp

+	for (auto line : lines) {
+		LOG(level::DEBUG, "inoreader_api::retrieve_auth: line = %s", line);
+		if (line.substr(0,5)=="Auth=") {
+			std::string auth = line.substr(5, line.length()-5);


You don't need the second argument to substr—it's npos by default, i.e. the whole "tail" of the string will be returned.

Just as with some of the things above, I just kept it this way based on the existing code because I could not find any contribution guidelines, but if you're open to improving the code, I'll certainly do it.

Oh, I'm all for improving the code! Modernize everything you see :)

Opened #28 regarding guidelines; thanks for a prod. Never knew what I should write there, but seems like I should make an effort and figure it out if I want to see more PRs coming my way.

Minoru · 2017-10-19T19:02:05Z

src/inoreader_api.cpp

+		json_object_object_get_ex(sub, "title", &node);
+		const char * title = json_object_get_string(node);
+
+		// Ignore URLs where ID start with given prefix - those never load,


Does this even apply to Inoreader?

I'll have to check actually.

Minoru · 2017-10-19T19:04:21Z

src/inoreader_api.cpp

+}
+
+bool inoreader_api::mark_all_read(const std::string& feedurl) {
+	std::string real_feedurl = feedurl.substr(strlen(INOREADER_FEED_PREFIX), feedurl.length() - strlen(INOREADER_FEED_PREFIX));


Same as earlier comment: no need for second argument here, substr got a useful default.

bartlibert · 2017-10-20T06:23:34Z

Some general answers:

I have about 8 years of professional c++ expercience, so don't worry about getting technical :)

I have a full-time job and a family, so I won't be able to respond to queries/bugs immediately, but if some delay is not an issue, I'm willing to have a look at those.

As for the oauth: I have no experience at all with that, but I'm willing to have a look.

As for the hardcoded tokens: even with oauth, they will still be there, as they are needed to identify the application itself. We might make them user-configurable, but I don't know it this will be useful.

coveralls · 2017-10-20T10:08:32Z

Coverage decreased (-1.3%) to 35.105% when pulling dfdb7c52eb248eafb0fb59b3c4820f74e6042bde on bartlibert:master into f40d5eb on newsboat:master.

coveralls · 2017-10-20T10:46:45Z

Coverage decreased (-0.6%) to 35.768% when pulling dfdb7c52eb248eafb0fb59b3c4820f74e6042bde on bartlibert:master into f40d5eb on newsboat:master.

Minoru · 2017-10-20T20:32:24Z

I have a full-time job and a family, so I won't be able to respond to queries/bugs immediately, but if some delay is not an issue, I'm willing to have a look at those.

Delay shouldn't be an issue; thanks! The volume is generally low, I think I saw less than 10 remote API-related bugs in Newsbeuter over 2 years.

As for the hardcoded tokens: even with oauth, they will still be there, as they are needed to identify the application itself. We might make them user-configurable, but I don't know it this will be useful.

Hmm, Inoreader's help page made me think otherwise. I'll look more closely into this in about 18 hours.

Minoru · 2017-10-21T16:08:19Z

doc/newsboat.txt

+~~~~~~~~~~~~~~~~~
+
+http://inoreader.com/[Inoreader] is another successor to Google Reader.
+Newsboat provides functionality to use The Inoreader as its


"The Inoreader"? I'm not sure the article is necessary there.

Minoru · 2017-10-21T16:10:00Z

doc/newsboat.txt

+so that it can authenticate with Inoreader.
+Note that this is NOT your login with your Google or Facebook account. If you
+use one of those to login to Inoreader, you have to create a username and
+password in Inoreader Preferences > Profile


Missing period at the end. Perhaps the "Preferences > Profile" bit should be in italics to make it clearer these are names of the menu items, not just general directions.

Minoru · 2017-10-21T16:13:44Z

doc/newsboat.txt

+After that, use these flags when you edit flags for an article, and these
+articles will be starred resp. shared.
+
+By default, newsboat also shows Inoreader "special feeds":


The names of special feeds are self-explanatory, so I'd just enumerate them without any explanations.

If you do want to make a list, add an empty line before it otherwise it isn't rendered as a list at all. (If you have asciidoc installed, you can make doc and preview the result in doc/xhtml/newsboat.html.)

Minoru · 2017-10-21T16:16:25Z

doc/newsboat.txt

+
+	inoreader-show-special-feeds no
+
+Inoreader's folders are converted into Newsboat tags. You can select and


This is already mentioned on lines 816-820. Let's combine these two paragraphs.

Minoru · 2017-10-21T16:20:30Z

include/inoreader_api.h

@@ -0,0 +1,48 @@
+#ifndef NEWSBOAT_INOREADER_API_H
+#define NEWSBOAT_INOREADER_API_H


Sorry, these two lines still miss the underscore at the end: it should be NEWSBOAT_INOREADER_API_H_.

Minoru · 2017-10-21T16:32:32Z

As for the hardcoded tokens: even with oauth, they will still be there, as they are needed to identify the application itself.

Upon re-reading their OAuth docs, I see you're right—we'll need to submit CLIENT_ID which we obtain by registering an app.

I agree with you that it doesn't make sense to make the ID configurable. I'll register an app and we'll use the token I get.

coveralls · 2017-10-23T15:35:43Z

Coverage decreased (-0.6%) to 35.768% when pulling 3b49e950229a1a4d1ae66bcc8cf541a1af2e2384 on bartlibert:master into b6c3eb5 on newsboat:master.

Minoru · 2017-10-23T17:13:19Z

Looks good!

Have you looked into OAuth yet? In the future, I expect to add more remote APIs that use it, so I looked around for a C++ library that will handle OAuth-specific bits for us, but the only standalone solution I could find (liboauthcpp) doesn't support OAuth 2.0. Looking forward to see what if find or come up with on this front. (I'm not hurrying you, I promise!)

bartlibert · 2017-10-24T07:00:24Z

I had a quick look at oauth, but did not find anything useful yet, so I fear it'll have to be implemented from scratch. I can have a look, but don't expect any results soon :(

coveralls · 2017-10-24T09:07:27Z

Coverage decreased (-0.6%) to 35.768% when pulling 814c0cd on bartlibert:master into 9f4a140 on newsboat:master.

Minoru · 2017-10-24T20:40:54Z

Okay, let's split OAuth into a separate task then (see #35). I'll definitely-certainly-absolutely will test this tomorrow and merge it. Thank you for the work you've already done!

Minoru · 2017-10-25T17:51:03Z

Merged! Thank you very much for implementing this! I've replaced the ID and key with mine, so you can delete yours now.

Did 70 requests just during testing; hope users won't set their reload-time to something silly like 1 minute. :)

bartlibert · 2017-10-26T06:08:00Z

Thanks for merging!
Let's hope the limitations are not causing any issues indeed. If people would run into trouble, giving them the possibility to use their own ID and key is a solution that should be considered I guess (albeit a sub par one).

Minoru · 2017-10-26T19:05:34Z

I'll just keep an eye on the stats and request an increase when we approach the limit. It also makes sense to check if we're using the API in the best possible way—perhaps we can get rid of some requests by caching things?

Minoru requested changes Oct 19, 2017

View reviewed changes

bartlibert force-pushed the master branch 2 times, most recently from 6edc829 to dfdb7c5 Compare October 20, 2017 09:49

Minoru mentioned this pull request Oct 20, 2017

Add contribution guidelines #28

Closed

2 tasks

Minoru requested changes Oct 21, 2017

View reviewed changes

bartlibert force-pushed the master branch from dfdb7c5 to 3b49e95 Compare October 23, 2017 14:42

Minoru added the enhancement This issue proposes a new feature or an improvement to an existing one label Oct 23, 2017

Bart Libert added 2 commits October 24, 2017 09:12

Update build dependencies

ad23389

Initial Inoreader implementation

814c0cd

bartlibert force-pushed the master branch from 3b49e95 to 814c0cd Compare October 24, 2017 08:33

Minoru mentioned this pull request Oct 24, 2017

Implement OAuth authorization for Inoreader #35

Open

Minoru merged commit 814c0cd into newsboat:master Oct 25, 2017

Minoru added this to the 2.10.2 milestone Mar 20, 2018

erikli mentioned this pull request Sep 30, 2018

Escape URLs for Inoreader, Old Reader #297

Merged

juhopp mentioned this pull request Aug 14, 2019

Crash when opening a url #189

Closed


		inoreader-show-special-feeds no

		Inoreader's folders are converted into Newsboat tags. You can select and

		@@ -0,0 +1,48 @@
		#ifndef NEWSBOAT_INOREADER_API_H
		#define NEWSBOAT_INOREADER_API_H

Initial inoreader implementation #22

Initial inoreader implementation #22

Conversation

bartlibert commented Oct 19, 2017

coveralls commented Oct 19, 2017 • edited

coveralls commented Oct 19, 2017

Minoru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlibert commented Oct 20, 2017

coveralls commented Oct 20, 2017 • edited

coveralls commented Oct 20, 2017 • edited

Minoru commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minoru commented Oct 21, 2017

coveralls commented Oct 23, 2017 • edited

Minoru commented Oct 23, 2017

bartlibert commented Oct 24, 2017

coveralls commented Oct 24, 2017 • edited

Minoru commented Oct 24, 2017

Minoru commented Oct 25, 2017

bartlibert commented Oct 26, 2017

Minoru commented Oct 26, 2017

coveralls commented Oct 19, 2017 •

edited

coveralls commented Oct 20, 2017 •

edited

coveralls commented Oct 20, 2017 •

edited

coveralls commented Oct 23, 2017 •

edited

coveralls commented Oct 24, 2017 •

edited