clarete / hackernews.el

Hacker News client for Emacs
GNU General Public License v3.0
249 stars 26 forks source link

Handle json.el and url.el errors on older Emacsen #42

Closed basil-conto closed 6 years ago

basil-conto commented 6 years ago

It has always been a pain trying to get hackernews to work on Emacs 23 (read: it didn't work :), so I did some digging and found GNU Emacs bug#23750. This should affect all Emacsen prior to version 25, and yet I don't see it materialising in version 24. Perhaps some other bug fix made it less apparent in the interim.

In brief, bug#23750 results in url-insert-file-contents returning a truncated payload, which in turn causes json-read to choke. The details are in this patch and the Emacs bug report.

The fact that no-one has reported this yet, however, suggests that none of our users are running Emacs 23 (which I would very much like to think). So my main question is, is it finally time to drop Emacs 23 support? I'm not too bothered either way, but having Emacs 24 will make #40 a bit less of a pain as well. :)

basil-conto commented 6 years ago

If Emacs 23 support is dropped, I will close this PR and only push the first paragraph of the Troubleshooting section, which I think is useful to mention either way.

I have never encountered any issues on Emacs 24, so I would rather not jump through hoops in hackernews--parse-json because of a theoretical possibility. We can always simply resurrect this PR if a user reports something.

basil-conto commented 6 years ago

Actually, neither the workaround in this PR nor the patch in bug#23750 suffice in making Emacs 23 work for me in general; url-http-wait-for-headers-change-function still chokes.

Anyone able to get it working? I'm now leaning quite strongly in favour of dropping Emacs 23 support.

basil-conto commented 6 years ago

I'm now leaning quite strongly in favour of dropping Emacs 23 support.

Which is a shame, because I just managed to get super-fast asynchronous url.el-backed retrieval working using only Emacs 23 features... :)

basil-conto commented 6 years ago

Another possibility is to maintain Emacs 23 compatibility using non-url.el retrieval, i.e. using curl/wget.

clarete commented 6 years ago

Hmm, that's an interesting question. I personally wouldn't feel the difference since I've been using Emacs from master for a while. I'm kinda neutral on that one call. Just out of curiosity, do know how support works for older Emacs versions? Like for how long people usually maintain a version of Emacs?

On another angle though, if you're already thinking about implementing other retrieval backends either way, it might not be that bad of a solution. Also, detecting which backend to use based on supported is better than faster but supported and faster can certainly be offered is a great way to not add version checks to the code.

I'm good either way, but as you said no body is using Emacs 23 too since no bugs were reported so far. So feel free to make the call!

basil-conto commented 6 years ago

I personally wouldn't feel the difference since I've been using Emacs from master for a while.

Me too ~(well, actually I'm currently on the emacs-26 branch because of a bug with the new early-init.el file)~, but hackernews.el never specified a minimum Emacs version so I've tried to maintain backward compatibility. :)

On another angle though, if you're already thinking about implementing other retrieval backends either way, it might not be that bad of a solution. Also, detecting which backend to use based on supported is better than faster but supported and faster can certainly be offered is a great way to not add version checks to the code.

I'm not sure exactly what you mean in your last sentence, but what I'm currently thinking of doing is adding a defcustom which determines how to perform retrieval. The user option's value will default to url.el asynchronous retrieval because it is pretty reliable on newer Emacsen, at least in my experience. On Emacs 23 it will default to asynchronous retrieval using wget or curl (#39), whichever is found first, in that order. This is because I can get neither synchronous nor asynchronous url.el retrieval to work reliably on Emacs 23, so I will not be able to maintain it.

The benefits of going this way are that all Emacs versions default to the faster asynchronous retrieval, and Emacs 23 actually works OOTB. The downsides are:

I'm good either way, but as you said no body is using Emacs 23 too since no bugs were reported so far. So feel free to make the call!

I'll try implementing the above and defer making the call for some day after that. :)

WDYT?

basil-conto commented 6 years ago

Sorry, I forgot to reply to a part of your message.

Just out of curiosity, do know how support works for older Emacs versions? Like for how long people usually maintain a version of Emacs?

IME, package authors just pick the oldest version of Emacs that they can realistically work with, or that is still quite popular at the time the package is first published. As such, it's kind of arbitrary. Usually it depends on the features/libraries the package depends on. For example, if a package author really can't live without cl-lib, then they can only go as far back as 24.3. If they depend on the new advice system, then they can only go as far back as 24.4. Other authors, particularly those who have been around longer, such as Drew Adams, try to maintain as much backward compatibility as possible in their packages.

Another metric is whichever Emacs version is widely distributed and considered stable, e.g. if Debian Stable is shipping Emacs 25 (as is currently the case), then some package authors see little point in supporting older versions than that.

In Emacs development itself, backward compatibility isn't really an issue (except when backporting bugfixes), as developers are free to use the latest features, but similar decisions are made on how long to keep obsolete aliases and deprecation warnings around for.

At the moment, obsolete aliases introduced in Emacs 23 are still considered viable because Debian OldOldStable packages it, and I think some version of RHEL might even have Emacs 22, but Emacs 21 and XEmacs obsolete aliases are safe to remove.

Coming back to hackernews - I think as long as I can install Emacs 23 binaries from a Debian repo (because good luck building it from source :) and successfully run and test hackernews, even if that means replacing url.el with external fetchers, there's no reason to drop support for it.

I think my eagerness from a few days ago was from frustration with url.el and the prospect of rewriting parts of hackernews to use shiny new Emacs 24 features. :P

clarete commented 6 years ago

Thank you for your very thorough explanation on the process. It's as usual a great opportunity for me to learn more about Emacs development! Also, thanks for highlighting the distribution factor too. That's so important.

About your previous comment. I think it's a great strategy and that looks like a very exciting update for 0.5.0. And sorry for not saying anything on that weird phrase related to version detection, I think your strategy is clear and I'm very glad that you were already planning on supporting those other backends, it makes the compatibility game very nice to handle!

basil-conto commented 6 years ago

As always, thanks for the feedback! I'll close this PR but push the first paragraph of the proposed Troubleshooting section to the README on master.