Overhaul and reimplement as major mode

basil-conto commented 7 years ago

Summary of user-visible changes

I have tried to make the commits as atomic / self-contained / explanatory as possible, so I recommend reviewing each one in turn. I decided against creating a string of smaller PRs as I find the changes herein to nicely sum up to a new major/minor package version. Nevertheless, I would be more than happy to oblige smaller PRs.

Create new major mode hackernews-mode derived from special-mode.
Support reading and switching between multiple Hacker News feeds, such as top stories, best stories, etc.
Afford more user customisation, e.g. via hooks and rendering format specs.
Extend all entrypoint commands to accept optional prefix argument specifying how many items to act on.
Actually echo help-echo button property (i.e. the button's URL) when moving around.
Run progress reporter when retrieving items.

Breaking changes

Rename defvar hackernews-top-story-limit to more general-purpose defcustom hackernews-items-per-page.
- Mitigation: define former as obsolete varalias of latter.
Rename hackernews-map to hackernews-mode-map as per major mode conventions.
- Mitigation: same as before - obsolete varalias.
Under hackernews-mode-map, g is now bound to hackernews-reload instead of hackernews.
- Mitigation: I would argue this is not a significant issue.
Switch from switch-to-buffer to higher-level and more customisable pop-to-buffer, which prefers switching to another window instead of reusing the current one.
- Mitigation: pop-to-buffer uses display-buffer as a subroutine, so users can customise display-buffer to open hackernews buffers as desired, e.g. via same-window-buffer-names.
Default to hackernews-load-more-stories not moving point.
- Mitigation: I would argue this is the correct default, as unprecedented moving of point is intrusive and non-Emacsy. Users can always customise hackernews-preserve-point to revert to old behaviour.

Before merging

Agree on naming of all new definitions, as well as the wording of their documentation.
Bump package version
- Agree on whether this PR comprises a major or minor version. Refactors with breaking changes are usually assigned a new major version, but the changes herein are arguably not that major/breaking, given the simple mitigations. I lack a strong opinion on this decision.
- Add :package-version keywords to all defcustoms accordingly.
- Add define-obsolete-variable-aliases accordingly for hackernews-top-story-limit->hackernews-items-per-page and hackernews-map->hackernews-mode-map.

basil-conto commented 7 years ago

Another to-do before merging is to update the README.

accidentalrebel commented 7 years ago

I tried it out and it works great! The ability to fetch the other hackernews feeds plus the detailed progress reporter are seemingly small but great improvements. I also don't see any problems with the other points you mentioned.

clarete commented 7 years ago

Same here, I didn't have the time to do any meaningful comment but I saw it running and you got me at the progress! That's amazing!!! Don't you just want push access to the project? So I don't block your progress? @basil-conto

I'm just on a time crunch at a other project and that's not your first very valuable contrib!!!

basil-conto commented 7 years ago

@accidentalrebel @clarete Thank you very much both for testing the changes and the positive feedback.

@clarete I would be very honoured by being granted push access, especially if it means sharing your workload. Either way, however, I would feel the need to seek some form of confirmation before merging anything in the future. :)

clarete commented 7 years ago

Done! Just invited you to get push access @basil-conto! I'm honored that you think the project is worth investing your time!! Can't wait to see this pushed to master! hackernews.el will get a new version soon after a while!!

basil-conto commented 7 years ago

@clarete Thank you very much. :) It's a pleasure using and hacking on this package.

I'll get back to this PR later tonight (UTC). In the meantime, is there anything I should be aware of re: MELPA? I'm asking because I've never had to deal with it before.

basil-conto commented 7 years ago

@clarete I've done the following in preparation for the release:

Minor Makefile aesthetics
Update copyright notices
Use standard package headers
Follow face naming conventions
Bump version to 0.4.0
- Update README
- Add obsolete aliases
- Add :package-version keywords to new defcustom options
- Add v0.4.0 tag

I will merge basil-conto:dev with clarete:master on Friday or following confirmation, whichever comes first.

clarete commented 7 years ago

I don't really know about the melpa thing either, IIRC I only uploaded it to marmalade. Doing some digging, I found this: https://github.com/melpa/melpa/blob/master/recipes/hackernews Which looks like what makes it available there.

When it comes to a "new release", I think it could be nice to maybe separate all the version number changes in the commit that will be tagged as that version. What do you think?

Also, given the extent of your contributions, please include your name as an author too!

\o/ That's very exciting!

clarete commented 7 years ago

The rest of the PR looks epic, thanks for referencing the Emacs Documentation in your changes! I feel like parts of it were talking to me! lol

‘Keywords’
  (...)
  The name of this field is unfortunate, since people often assume
  it is the place to write arbitrary keywords that describe their
  package, rather than just the relevant Finder keywords.

clarete commented 7 years ago

One correction, I missed that the last commit does pretty much what I suggested! I'm 👍 with the PR after the author change! \o/

basil-conto commented 7 years ago

I only uploaded it to marmalade

Ah, I always forget about Marmalade and mix it up with MELPA. Indeed, the latter gets updated automatically by tracking the version-controlled repository, whereas the former requires manual upload. It looks like https://marmalade-repo.org/ is stagnating, though.

the last commit does pretty much what I suggested

Great, was just about to ask. :)

I feel like parts of it were talking to me

I always get that feeling when it comes to GNU/FSF documentation. :)

I'm 👍 with the PR after the author change

OK, will do that, rebase and release. Thanks everyone for all the quick feedback and support. :)

accidentalrebel commented 7 years ago

I just updated from Melpa and everything works. Good job on this new update!

basil-conto commented 7 years ago

@accidentalrebel Thanks! Already found and fixed the first bug after seeing your comment. :)

clarete / hackernews.el