hyakt / emacs-dashboard-hackernews

Display a topstories of Hacker News on Dashboard.
18 stars 4 forks source link

Fixed wrong number of arguments issue #4

Closed themkat closed 2 years ago

themkat commented 2 years ago

Currently, emacs-dashboard-hackernews crashes the entire dashboard when starting due to a wrong number of arguments issue. This is because of a fix in dashboard: https://github.com/emacs-dashboard/emacs-dashboard/pull/344

As per my understanding, it works the same with just the start-argument removed (defaulting to 0).

paulmyr commented 2 years ago

Tried to patch my local version with this fix - resulted in the following error. Wrong type argument: integer-or-marker-p, nil On the other hand, I've started using emacs yesterday, so it maybe some mistake in my configuration

themkat commented 2 years ago

Tried to patch my local version with this fix - resulted in the following error. Wrong type argument: integer-or-marker-p, nil On the other hand, I've started using emacs yesterday, so it maybe some mistake in my configuration

Did you use the newest emacs-dashboard version? I only started getting it once I updated dashboard very recently

paulmyr commented 2 years ago

Did you use the newest emacs-dashboard version? I only started getting it once I updated dashboard very recently

I'm using the current melpa version: 20220409.620 which seems to be the newest one

themkat commented 2 years ago

Then it is indeed weird 😨 This is the code relating to this change in the latest melpa version, and as we can see it is obvious that the change in this PR needs to be done when the start parameter is removed. Maybe there are some more that should be changed?

I use the same version on both my machines, and have been running my change for weeks now. I'm not getting any issues, but that may also be due to my combination or packages. (many things like the different other dependencies can interfere here).

Could you confirm where the error is coming from with a stacktrace or similar? (i.e, starting Emacs with -debug-init and/or use M-x toggle-debug-on-error if the error happens later).

paulmyr commented 2 years ago

Then it is indeed weird fearful This is the code relating to this change in the latest melpa version, and as we can see it is obvious that the change in this PR needs to be done when the start parameter is removed. Maybe there are some more that should be changed?

I use the same version on both my machines, and have been running my change for weeks now. I'm not getting any issues, but that may also be due to my combination or packages. (many things like the different other dependencies can interfere here).

Could you confirm where the error is coming from with a stacktrace or similar? (i.e, starting Emacs with -debug-init and/or use M-x toggle-debug-on-error if the error happens later).

The only information I could get is the following: dashboard-maximum-section-length: Wrong type argument: integer-or-marker-p, nil my init looks like this:

(use-package dashboard-hackernews :load-path "/home/USERNAME/.emacs.d/patched_elpa/dashboard-hackernews-20190109.205" :init (setq dashboard-items '((hackernews . 10))))

When using "M-x dashboard-refresh-buffer" my dashboard reloads and instead of my usual sections, I get two Hackernews section displayed, one section as configured (10 entries), then another one filling the display.

Starting with --debug-init didn't display any different information.

themkat commented 2 years ago

First of all, the error you are describing with two Hackernews sections are NOT coming from this PR. It is weird if you have not experienced it before with the config above before.... I will explain it below 🙂

First, the reason I have not experienced it. I control the order that use-package loads the packages in, using :after. That has probably masked the error somehow due to dashboard being loaded after dashboard-hackernews. (as well as the setq expression, more about that next).

You are setting the dashboard-items variable in the init-section, which means it is set before the contents of dashboard-hackernews.el is loaded. Because of this line, another hackernews entry is added to the list without a size. This probably causes the above error as well, because in my guess the size of the block is set to nil implicitly which the max function is not meant to handle.

The error you got was indeed confusing. Should we maybe remove this add-to-list statement? Then it won't be added automatically, but I guess most people will add it manually with the amount of items they want anyway?

paulmyr commented 2 years ago

First of all, the error you are describing with two Hackernews sections are NOT coming from this PR. It is weird if you have not experienced it before with the config above before.... I will explain it below slightly_smiling_face

First, the reason I have not experienced it. I control the order that use-package loads the packages in, using :after. That has probably masked the error somehow due to dashboard being loaded after dashboard-hackernews. (as well as the setq expression, more about that next).

You are setting the dashboard-items variable in the init-section, which means it is set before the contents of dashboard-hackernews.el is loaded. Because of this line, another hackernews entry is added to the list without a size. This probably causes the above error as well, because in my guess the size of the block is set to nil implicitly which the max function is not meant to handle.

I changed my config to the following: (use-package dashboard-hackernews :load-path "/home/mayerpa/.emacs.d/patched_elpa/dashboard-hackernews-20190109.205" :after dashboard :config (setq dashboard-items '((hackernews . 10))) ) I still get the same error message. Only when removing the I changed my config to the following: :config (setq dashboard-items '((hackernews . 10)))` part entirely, dashboard loads as usually (not displaying any hacernews section)

The error you got was indeed confusing. Should we maybe remove this add-to-list statement? Then it won't be added automatically, but I guess most people will add it manually with the amount of items they want anyway?

Tbh, because elisp looks like some black magic fckery to me and since I don't really have time to dig into this at the moment, it's probably best to just let this sit for a while. I kindly appreciate your help tho.

themkat commented 2 years ago

First of all, the error you are describing with two Hackernews sections are NOT coming from this PR. It is weird if you have not experienced it before with the config above before.... I will explain it below slightly_smiling_face First, the reason I have not experienced it. I control the order that use-package loads the packages in, using :after. That has probably masked the error somehow due to dashboard being loaded after dashboard-hackernews. (as well as the setq expression, more about that next). You are setting the dashboard-items variable in the init-section, which means it is set before the contents of dashboard-hackernews.el is loaded. Because of this line, another hackernews entry is added to the list without a size. This probably causes the above error as well, because in my guess the size of the block is set to nil implicitly which the max function is not meant to handle.

I changed my config to the following: (use-package dashboard-hackernews :load-path "/home/mayerpa/.emacs.d/patched_elpa/dashboard-hackernews-20190109.205" :after dashboard :config (setq dashboard-items '((hackernews . 10))) ) I still get the same error message. Only when removing the I changed my config to the following::config (setq dashboard-items '((hackernews . 10)))` part entirely, dashboard loads as usually (not displaying any hacernews section)

The error you got was indeed confusing. Should we maybe remove this add-to-list statement? Then it won't be added automatically, but I guess most people will add it manually with the amount of items they want anyway?

Tbh, because elisp looks like some black magic fckery to me and since I don't really have time to dig into this at the moment, it's probably best to just let this sit for a while. I kindly appreciate your help tho.

That is indeed weird! I don't get the error message on any of my machines (both Macs atm) 🙁 I get the issue you talked about with two hackernews sections like I mentioned earlier, but no error message. Let's see if someone else is reporting any issues, or if the maintainer have any comments 🙂

elisp and the order of loading is indeed confusing at times! Have been using Emacs for over ten years now, and sometimes I still get confused on weird errors happening because of load-order 😱

paulmyr commented 2 years ago

Solved the problem. I went and copied your config from your blog, and it didn't crash my application anymore. I don't get any errors and it works other sections are displayed as I wanted. However, to display the hackernews sections, I need to M-x dashboard-refresh-buffer. Then everything works as intended. If you know how to fix this timing issue automatically (can I automatically execute this function when entering dashboard-mode?), I would greatly appreciate it.

themkat commented 2 years ago

Hmm. I don't need to do anything to refresh it. You could try to add a :config (dashboard-refresh-buffer) section to your dashboard config? Thats the only solution I can think of at the moment. Tried it just to be sure, and indeed I see dashboard refresh during startup. You can also use the key g when in the dashboard buffer to refresh it manually 🙂 (you probably knew that, but a good thing cant be shared too much!)

Super happy that you got something useful from my blog! 😄

carwin commented 2 years ago

So the chance of anything happening with this seems PR seems pretty low considering there's another PR from 2019 just waiting to be reviewed or merged.

I ran into some minor bug that needed tweaking and ended up just forking the project, with the intent of pulling in stuff like this. Mainly so I could simplify my config and not have to deal with straight.el patching stuff. If you'd like to submit this PR to my fork I'll be happy to merge it so you can use my repo as the target with your package manager.

https://github.com/carwin/dashboard-hackernews.el

@hyakt is still active on GH so maybe he'll come back to the project eventually, but until then I'm happy to be the baby-sitter / interim maintainer.

themkat commented 2 years ago

So the chance of anything happening with this seems PR seems pretty low considering there's another PR from 2019 just waiting to be reviewed or merged.

I ran into some minor bug that needed tweaking and ended up just forking the project, with the intent of pulling in stuff like this. Mainly so I could simplify my config and not have to deal with straight.el patching stuff. If you'd like to submit this PR to my fork I'll be happy to merge it so you can use my repo as the target with your package manager.

https://github.com/carwin/dashboard-hackernews.el

@hyakt is still active on GH so maybe he'll come back to the project eventually, but until then I'm happy to be the baby-sitter / interim maintainer.

Yeah. Seems like he is not very active here anymore...

Nice 🙂 Will star your repo to remember it for the future. Seems like the issue which this PR fixes is already fixed in your fork. Will remember your repo for future issues if any 🙂

carwin commented 2 years ago

Seems like the issue which this PR fixes is already fixed in your fork

Hah! I'm realizing now I was looking at https://github.com/emacs-dashboard/emacs-dashboard/pull/344 and thinking it was this PR's commit. Typical Monday brain.

At any rate, I'll still plan to be the placeholder maintainer. I want to go through #1 this weekend and port it over since I very much would like #2 sorted out, so if you or @jkdufair get the urge to do it sooner, go for it and I'll get it merged :)

hyakt commented 2 years ago

Sorry for not being able to participate in the discussion. As assumed, I am currently unable to perform maintenance this repository.

@themkat @carwin If you are interested, I have invited you as a collaborator and would be happy to help with maintenance.

jkdufair commented 2 years ago

Not currently using dashboard but glad to help here if need be.

themkat commented 2 years ago

@carwin , I see that both my new PR here and the old one from 2019 will both fix the current issue. I have not tested the older code yet, and I'm a little bit unsure if it will cause new issues (it is quite old after all). Should we maybe merge my PR and look into adapting the older code later?

carwin commented 2 years ago

Firstly, thanks @hyakt!

@themkat - sounds like a plan to me!