Keep track of visited links

[matthew-piziak]

This keeps track of visited item IDs, and differentiates visited links with faces.

Links are persisted to hackernews-visited-ids-file.

Closes #33.

[basil-conto]

After this PR is merged, we should also submit one to https://github.com/emacscollective/no-littering so that the new user configuration file is handled correctly for that package's users (myself not included).

[matthew-piziak]

@basil-conto Thanks! This is like a master class on how to write Emacs packages. I've gone and implemented some of your feedback.

[basil-conto]

@matthew-piziak After my latest comments have been addressed, and before merging this PR, would you mind if I pushed a commit or two to your branch? I have some ideas for how to make data loss less likely when reading from / writing to hackernews-visited-links-file.

[alphapapa]

I have some ideas for how to make data loss less likely when reading from / writing to hackernews-visited-links-file.

That sounds like the kind of thing that would be worthy of a blog post. Or perhaps a "best practices" entry in https://github.com/alphapapa/emacs-package-dev-handbook, if you would be interested in contributing. :)

[basil-conto]

I have some ideas for how to make data loss less likely when reading from / writing to hackernews-visited-links-file.

That sounds like the kind of thing that would be worthy of a blog post.

Sorry, I don't blog, and I'm not sure the ideas I have are as noteworthy as you suggest. :) All I'm thinking of is merging the ID sets on disc with those in memory before writing to disc again. The same functionality could also be used for the initial read from disc, as the union of a set with the empty set is simply the former set. What's more, the user could be advised to invoke this function interactively after they fix whatever gave rise to any file-errors in the first place, so as to avoid overwriting any valid data on exit.

Or perhaps a "best practices" entry in https://github.com/alphapapa/emacs-package-dev-handbook, if you would be interested in contributing.

Thanks, this handbook sounds like a good idea and I wasn't aware of it. :) I'll be sure to look into it when I find the time.

[matthew-piziak]

@basil-conto Thank you! This is teaching me a lot.

Sorry about the tabs. I forgot I had aggressive-indent-mode on and it was tabifying everything.

I've edited in your suggestions but it's a bit broken. I'll debug it tomorrow.

Feel free to make a recursive PR into my branch. Or if it helps your workflow I'll just give you commit permission to my fork.

[basil-conto]

I've edited in your suggestions but it's a bit broken.

How so? The only issue I can see is the name mismatch between

(defun hackernews-visited-links-save ()
  ...)

and

(add-hook 'kill-emacs-hook #'hackernews-save-visited-links)

I can't notice any obvious issues after fixing this typo.

FWIW, I'm using the following "life hack" for faster link visit testing:

(define-key hackernews-button-map "v"
  (lambda ()
    (interactive)
    (hackernews--visit (point) #'ignore)))

[basil-conto]

Feel free to make a recursive PR into my branch. Or if it helps your workflow I'll just give you commit permission to my fork.

I think I already have push access to your fork as a collaborator on this project. Only if you had created the PR against your fork's master branch would I not have had push access.

I think I have the ID set merging I described earlier ready now, so I'll push it along with the typo fix to your feature branch. I'd be grateful if you could review this then.

Just so I know, do you intend to squash or reword any of your commits prior to merging the PR, or do you prefer to keep the complete development history?

[basil-conto]

I think I have the ID set merging I described earlier ready now, so I'll push it along with the typo fix to your feature branch.

Spoke too soon, have to run now. I'll push these changes later this evening (UTC+0300).

[matthew-piziak]

Just so I know, do you intend to squash or reword any of your commits prior to merging the PR, or do you prefer to keep the complete development history?

I am perfectly happy with either workflow—I normally follow repo convention. If you'd prefer a squashed history just let me know and I'll perform the rebase.

[basil-conto]

I am perfectly happy with either workflow

Same.

I normally follow repo convention.

I'm not aware of any established conventions in this repo. :)

If you'd prefer a squashed history just let me know and I'll perform the rebase.

Up to you. After I push my commits and you give them the thumbs-up, I'll wait a few days in case @clarete or anyone else has feedback and merge the branch as it is.

[matthew-piziak]

@basil-conto Great. Let's just do that. If we do decide to squash I'll make sure you're done with the branch before I obliterate history.

[basil-conto]

@matthew-piziak I'm done for now, please have a look at the commits I pushed to your branch and let me know what you think.

[matthew-piziak]

@basil-conto All looks good! Binding a "mark as read" function to "r" might be a good idea. It seems to be a common pattern.

[basil-conto]

Thanks.

Binding a "mark as read" function to "r" might be a good idea. It seems to be a common pattern.

Good idea, would you like to add it?

[matthew-piziak]

@basil-conto Sure. How's this look?

[basil-conto]

@matthew-piziak Apart from the minor nitpick, LGTM.

[basil-conto]

@clarete I intend to merge this next Saturday 1st September if you have no comments before then.

[clarete]

No objections at all, thanks a lot for the neat code and honestly, this is one of the most thorough reviews that I've seen in a few years. Thanks a lot for all the work, this is what poetry looks like when it's encoded in Emacs-Lisp! <3