This way of prepending all logs within this function with the function name gives a build warning 431 (see https://luacheck.readthedocs.io/en/stable/warnings.html). How much of a problem is that warning?

Reviewed 1 of 2 files at r8.
Reviewable status: 1 of 2 files reviewed, 34 unresolved discussions (waiting on @harmtemolder and @poire-z)

plugins/wallabag.koplugin/main.lua line 600 at r9 (raw file):

            if #article_list == self.articles_per_sync then
                logger.dbg("Wallabag:getArticleList: #article_list == self.articles_per_sync ==", self.articles_per_sync)
                break -- exit for loop

useless comment

plugins/wallabag.koplugin/main.lua line 612 at r9 (raw file):

--- Remove all the articles from the list containing one of the ignored tags.
-- @tparam tab article_list Array containing a JSON formatted list of articles
-- @treturn tab Same array, but without any articles that contain an ignored tag.

We write table in full in such scenarios.

plugins/wallabag.koplugin/main.lua line 781 at r9 (raw file):

                    logger.dbg("Wallabag:callAPI: JSON downloaded")
                    -- Only enable this log when needed, the output can be large
                    -- logger.dbg("result =", result)

Undesirable change

plugins/wallabag.koplugin/main.lua line 870 at r9 (raw file):

        UIManager:forceRePaint()

        for i, article in ipairs(articles) do -- Do not use `_`

Useless comment

plugins/wallabag.koplugin/main.lua line 964 at r9 (raw file):

    if self.upload_queue and next(self.upload_queue) ~= nil then
        local msg = T(_("Uploading %1 articles from queue…"), #self.upload_queue)

The old string Adding articles from queue… is better. No articles are being uploaded, only some URLs are added ("uploaded" in some technical sense) to Wallabag.

plugins/wallabag.koplugin/main.lua line 981 at r9 (raw file):

    if not quiet then
        local msg = T(_("Uploaded %1 articles from the queue to wallabag"), count)

Also uploaded.

Plurals need to be double wrapped for localization. http://koreader.rocks/doc/modules/gettext.html#ngettext

plugins/wallabag.koplugin/main.lua line 1016 at r9 (raw file):

                    logger.dbg("Wallabag:processRemoteDeletes: deleting", local_id, "at", entry_path)
                    count = count + self:deleteLocalArticle(entry_path)
                end -- if self.use_local_archive

Useless comment

plugins/wallabag.koplugin/main.lua line 1020 at r9 (raw file):

                logger.dbg("Wallabag:processRemoteDeletes: local_id", local_id, "found in remote_ids; not archiving/deleting")
            end -- if not remote_article_ids[ id ]
        end -- if entry ~= . and entry ~= ..

Useless comment

plugins/wallabag.koplugin/main.lua line 1045 at r9 (raw file):

    if self.archive_finished or self.archive_read or self.archive_abandoned then
        local info = InfoMessage:new{ text = _("Uploading local article statuses…") }

uploading

should be submitting or syncing

plugins/wallabag.koplugin/main.lua line 1240 at r9 (raw file):

        util.makePath(self.archive_directory)
        UIManager:show(InfoMessage:new{
            text = T(_("Created the archive directory at %1."), self.archive_directory),

Useless info message.

plugins/wallabag.koplugin/main.lua line 1416 at r9 (raw file):

function Wallabag:setArticlesPerSync(touchmenu_instance)
    self.articles_dialog = InputDialog:new{
        title = _("Number of articles to download"),

Something like "to store/keep locally" seems clearer.

plugins/wallabag.koplugin/main.lua line 1515 at r9 (raw file):

function Wallabag:onAddWallabagArticle(article_url)
    if not NetworkMgr:isOnline() then
        self:addToUploadQueue(article_url)

Definitely not a fan of changing the function name either. It's a queue to download. The fact that it does so by uploading a few bytes is an implementation detail.

plugins/wallabag.koplugin/main.lua line 1517 at r9 (raw file):

        self:addToUploadQueue(article_url)
        UIManager:show(InfoMessage:new{
            text = T(_("Article added to upload queue:\n%1"), BD.url(article_url)),

Definitely should not be upload queue in anything user facing.

plugins/wallabag.koplugin/main.lua line 1572 at r9 (raw file):

    NetworkMgr:runWhenOnline(connect_callback)

    -- stop propagation

useless comment

plugins/wallabag.koplugin/main.lua line 1590 at r9 (raw file):

    end

    -- stop propagation

useless comment

Reviewable status: 0 of 2 files reviewed, 30 unresolved discussions (waiting on @Frenzie and @poire-z)

plugins/wallabag.koplugin/main.lua line 600 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 612 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 781 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 870 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 964 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 981 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1016 at r9 (raw file):

I added these when I was counting ends and read https://github.com/koreader/koreader-base/wiki/Coding-style#end-terminator, but I'm happy to remove them if you think they are useless

plugins/wallabag.koplugin/main.lua line 1045 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1079 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1240 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1416 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1517 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1572 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1590 at r9 (raw file):

Done.

Reviewable status: 0 of 2 files reviewed, 30 unresolved discussions (waiting on @Frenzie and @poire-z)

plugins/wallabag.koplugin/main.lua line 1515 at r9 (raw file):

I agree to not calling it "upload" for anything user facing, so I've changed that.

But regarding the variable and function names: trying to understand the internal functionality of this plugin and the flow through it, it was very confusing for me that something called "download_queue" was not used to download anything:

• If there is a network connection, the article is added to the remote wallabag directly, but not downloaded to KOReader yet until the next manual download
• If there is no network connection, the article is added to the queue, to be added to the remote wallabag on the next manual upload or complete sync

Only after adding the articles to the remote wallabag will they be downloaded, like any other new article added from anywhere else. The "download_queue" variable is not part of that process.

Reviewable status: 0 of 2 files reviewed, 30 unresolved discussions (waiting on @Frenzie and @poire-z)

Reviewed 1 of 2 files at r10.
Reviewable status: 1 of 2 files reviewed, 19 unresolved discussions (waiting on @harmtemolder and @poire-z)

plugins/wallabag.koplugin/main.lua line 964 at r9 (raw file):

Adding 1 article from queue…

plugins/wallabag.koplugin/main.lua line 981 at r9 (raw file):

from queue

plugins/wallabag.koplugin/main.lua line 1016 at r9 (raw file):

I'm not saying each and every one is necessarily useless (that being said, I'm not sure if this file even contains anything that resembles a "large block") but if self.use_local_archive for example is a mere 4 lines up. That makes it a double whammy maintenance nightmare: extremely distracting and guaranteed to be outdated soon.

In short, since they were added indiscriminately I'd also remove them indiscriminately, yes.

plugins/wallabag.koplugin/main.lua line 1515 at r9 (raw file):

I think I understand your point about download, although I don't agree with it and that's how I feel about the word upload. What about something like pending queue or sync queue?

plugins/wallabag.koplugin/main.lua line 208 at r10 (raw file):

                                        path = filemanagerutil.abbreviate(self.directory)
                                    end
                                    return T(_("Download directory: %1"), BD.dirpath(path))

Download folder

plugins/wallabag.koplugin/main.lua line 329 at r10 (raw file):

                            },
                            {
                                text = _("Move to archive directory instead of deleting"),

archive folder

plugins/wallabag.koplugin/main.lua line 461 at r10 (raw file):

-- Do not request a new token if the saved one is valid for more than 5 minutes.
function Wallabag:getBearerToken()
    local function is_empty(s)

isEmpty

plugins/wallabag.koplugin/main.lua line 582 at r10 (raw file):

            -- No error occurred, but no JSON was returned either
            logger.warn("Wallabag:getArticleList: requesting page", page, "did not return anything")
            UIManager:show(InfoMessage:new{ text = _("Requesting article list did not return anything.") })

Is that an accurate change? nil and a JSON that is "empty" are two very different things.

plugins/wallabag.koplugin/main.lua line 866 at r10 (raw file):

                #articles
            ),
            timeout = 5

timeout = 3

plugins/wallabag.koplugin/main.lua line 886 at r10 (raw file):

                        #articles
                    ),
                    timeout = 5

timeout = 3

plugins/wallabag.koplugin/main.lua line 912 at r10 (raw file):

        logger.info("Wallabag:downloadArticles: - queue_count =", queue_count)
        if queue_count > 0 then
            msg = msg..T(_("\n- added from queue: %1"), queue_count)

concatenate the \n, don't bother translators with it

Though the part about not being too clever also goes. For localization it's generally preferable to write it out in full.

As a user I don't care about skipped at all tbh, that's almost always >0, and as a user I'm definitely not convinced that dynamically changing the message so everything is always somewhere else is desirable.

I'd just skip all the cleverness as a single message which clearly says 0 to serve users better, and it's better for translators as an added bonus.

plugins/wallabag.koplugin/main.lua line 931 at r10 (raw file):

        if del_count_local > 0 then
            if self.use_local_archive then
                msg = msg..T(_("\n- archived in KOReader: %1"), del_count_local)

cleverness works for these, but note the \n concat

plugins/wallabag.koplugin/main.lua line 1113 at r10 (raw file):

        if self.delete_instead then
            msg = msg..T(_("\n- deleted from Wallabag: %1"), count_remote)

\n concat

Reviewable status: 1 of 2 files reviewed, 19 unresolved discussions (waiting on @Frenzie and @poire-z)

plugins/wallabag.koplugin/main.lua line 964 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 981 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1016 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1020 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1515 at r9 (raw file):

How do you feel about "offline_queue", because it's only used when adding articles when offline?

plugins/wallabag.koplugin/main.lua line 208 at r10 (raw file):

So for my understanding: I should use "folder" in anything user facing and "directory" in variable names?

plugins/wallabag.koplugin/main.lua line 461 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 582 at r10 (raw file):

Unfortunately, https://app.wallabag.it/api/doc/ does not explain if an empty JSON will ever be returned from the /api/entries endpoint, but you're right, my wording here might not be correct.

I kept this check for nil because it was there before, but seeing that I've adapted callAPI to always give true/false as its first return value, it will never trigger: true is only ever returned when a valid JSON was returned by the wallabag API (or a filepath was provided, which is not the case here). So this check can be removed entirely.

plugins/wallabag.koplugin/main.lua line 866 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 886 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 912 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 931 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1113 at r10 (raw file):

Done.

Reviewable status: 1 of 2 files reviewed, 8 unresolved discussions (waiting on @harmtemolder and @poire-z)

plugins/wallabag.koplugin/main.lua line 1515 at r9 (raw file):

Yes, indeed, excellent. It's an offline (message/article) queue. 👍

plugins/wallabag.koplugin/main.lua line 208 at r10 (raw file):

Close enough. It's indeed folder for user-facing. For the code it matters less but underlying C and callers/wrappers tend to use dir. What's clearer can be contextual, which also includes whatever it's currently named. (In the sense of don't randomly change things for weak reasons, at least mixed in with other changes.)

plugins/wallabag.koplugin/main.lua line 582 at r10 (raw file):

Depending on the actual behavior it should then either be moved one up as not ok and result == nil or stay here as something along the general lines of #result == 0.

plugins/wallabag.koplugin/main.lua line 344 at r11 (raw file):

                                        path = filemanagerutil.abbreviate(self.archive_directory)
                                    end
                                    return T(_("Archive directory: %1"), BD.dirpath(path))

Archive folder

plugins/wallabag.koplugin/main.lua line 415 at r11 (raw file):

                    {
                        text = _("Send review as tags"),
                        help_text = _("This allows you to write tags in the review field, separated by commas, which will be uploaded to the article on Wallabag."),

added to or applied to the article

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @harmtemolder)

plugins/wallabag.koplugin/main.lua line 876 at r12 (raw file):

        info = InfoMessage:new{
            text = T(
                _("Got a list of %1 articles…"),

Received

plugins/wallabag.koplugin/main.lua line 1597 at r12 (raw file):

end

function Wallabag:addToUploadQueue(article_url)

addToOfflineQueue

plugins/wallabag.koplugin/main.lua line 1069 at r12 (raw file):

            {
                text = self.server_url,
                --description = T(_("Server URL:")),

Don't randomly remove these.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Frenzie)

plugins/wallabag.koplugin/main.lua line 1515 at r9 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 208 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 329 at r10 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 582 at r10 (raw file):

I have tried different calls to the wallabag API using Insomnium to see what is returned:

• There is a pages parameter that we can use to stop us from having to rely on a 404 error to determine the last page (see below).
• I do propose to leave the check for 404 in, just in case. But split in two:
  • If no articles were returned from previous pages, alert the user of the 404 error, because something else might be wrong
  • If articles were returned from previous pages, return those without alerting, because for whatever reason we might have blown past the last page
• I did find a case where 0 items were returned: when a non-existing tag was used to filter. I was thinking of changing the last message to something like "Requesting article list did not return anything. Are your tag filters correctly set up?", but there might be other edge cases. What do you think?

plugins/wallabag.koplugin/main.lua line 344 at r11 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 415 at r11 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 876 at r12 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1069 at r12 (raw file):

Done.

plugins/wallabag.koplugin/main.lua line 1597 at r12 (raw file):

Done.

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @harmtemolder)

plugins/wallabag.koplugin/main.lua line 582 at r10 (raw file):

If Wallabag doesn't distinguish between a tag with no articles associated and a non-existent tag I wouldn't editorialize it.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @harmtemolder)

plugins/wallabag.koplugin/main.lua line 582 at r10 (raw file):

Good point. I'll leave it as is

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @harmtemolder)