-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
passing in a feed as a flag requires a manual feed refresh #49
Comments
Hmm, was looking at this issue thinking I could look into it, but I actually can't get RSS feeds to work from |
@kanielrkirby That's been my experience. |
Hm, I'll open a separate issue for this, it seems to be a regression. |
So what I'm seeing is that the issue is this, in Commands#TUI(): its, err := c.GetAllFeeds()
if err != nil {
return fmt.Errorf("commands List: %w", err)
}
var errorItems []ErrorItem
// if no feeds in store or we have preview feeds, fetchAllFeeds
if len(its) == 0 || len(c.config.PreviewFeeds) > 0 {
_, errorItems, err = c.fetchAllFeeds()
if err != nil {
return fmt.Errorf("[commands.go] TUI: %w", err)
}
// refetch for consistent data across calls
its, err = c.GetAllFeeds()
if err != nil {
return fmt.Errorf("[commands.go] TUI: %w", err)
}
} What ends up happening is that you'll There's a few options here, and what seems simplest is to just break up the logic. For previewing (passing in Example change that seems to fix both issues at first glance: var errorItems []ErrorItem
// if no feeds in store or we have preview feeds, fetchAllFeeds
if len(c.config.PreviewFeeds) > 0 {
its, errorItems, err = c.fetchAllFeeds()
if err != nil {
return fmt.Errorf("[commands.go] TUI: %w", err)
}
} else if len(its) == 0 {
_, errorItems, err = c.fetchAllFeeds()
if err != nil {
return fmt.Errorf("[commands.go] TUI: %w", err)
}
// refetch for consistent data across calls
its, err = c.GetAllFeeds()
if err != nil {
return fmt.Errorf("[commands.go] TUI: %w", err)
}
} I'll hold off on a PR until I get some feedback from @guyfedwards, as I'm not sure of the implications of this change by itself, if any. |
@kanielrkirby It works for me! |
Only thing to ensure here is taht we aren't saving previewFeeds to the db, they should be cleaned up by I have considered removing the previewFeeds feature as wasn't really sure of it's benefit over just adding an entry in the yaml and removing it, might be worth discussing but seems like there are some users using it at least. |
I think I like preview, as it provides a little flexibility, "one-shot" checking. But maybe it would good to separate that logic completely from the current logic for the store. Just a thought I'm having. With that said, I think it's working as expected, as |
from #42
The text was updated successfully, but these errors were encountered: