Add limit and pagination to feeds #2980
Conversation
crates/routes/src/feeds.rs
Outdated
| @@ -173,6 +198,28 @@ async fn get_feed( | |||
| ) | |||
| } | |||
|
|
|||
| fn get_limit_and_page(req: &HttpRequest) -> (i64, i64) { | |||
There was a problem hiding this comment.
Could return a struct here if more query string params are added, but I think a tuple suffices for now.
crates/routes/src/feeds.rs
Outdated
| } | ||
| _ => {} | ||
| } | ||
| }); |
There was a problem hiding this comment.
This is not how you should read params.
There was a problem hiding this comment.
Oof yea, totally missed that.
Suppose if we need posts from 5 to 15, we can use the |
|
@tillcash So if the |
|
Our database impl only supports page/limit params. Supporting |
6-15 |
|
@Nutomic I was able to leverage |
|
Also, I believe something unrelated to the PR is failing in the test pipeline. I've noticed it in some other pipelines as well having to do with the comment view (ex: https://woodpecker.join-lemmy.org/LemmyNet/lemmy/pipeline/70/12) |
crates/routes/src/feeds.rs
Outdated
| @@ -38,6 +38,8 @@ const RSS_FETCH_LIMIT: i64 = 20; | |||
| #[derive(Deserialize)] | |||
| struct Params { | |||
| sort: Option<String>, | |||
| limit: Option<i64>, | |||
| after: Option<i64>, | |||
There was a problem hiding this comment.
Don't use after. Use limit and page like all the other objects.
There was a problem hiding this comment.
Cool, reverted back to using page
jschne88
force-pushed
the
add-limit-and-pagination-to-feeds
branch
from
e4d6ef8 to
c28f104
Compare
crates/routes/src/feeds.rs
Outdated
| let sort_type = get_sort_type(&info).map_err(ErrorBadRequest)?; | ||
| let limit = info.limit.unwrap_or(RSS_FETCH_LIMIT); | ||
| let page = info.page.unwrap_or(1); | ||
| Ok(get_feed_data(&context, ListingType::All, sort_type, limit, page).await?) |
There was a problem hiding this comment.
Would be cleaner if you move this stuff into a helper function in impl Params.
There was a problem hiding this comment.
Good idea - pushed this change
|
@dessalines Is there a way to manually trigger a rerun of the CI pipeline with woodpecker? I fixed the clippy issue but for some reason that step failed again (perhaps a cacheing issue?). Could do an empty commit but that's messy IMO. |
Nutomic
force-pushed
the
add-limit-and-pagination-to-feeds
branch
from
b493074 to
0b6f006
Compare
|
Yes its a known issue that woodpecker is sometimes building old versions of the code. I opened an issue for that some time ago, but cant find that right now. Not sure if you can restart builds but i did that just now. |
| #[tracing::instrument(skip_all)] | ||
| async fn get_feed_user( | ||
| pool: &DbPool, | ||
| sort_type: &SortType, | ||
| limit: &i64, |
There was a problem hiding this comment.
Surprised clippy isn't saying these are unecessary. It passed the CI tho anyway.
Nutomic
force-pushed
the
add-limit-and-pagination-to-feeds
branch
from
9a80277 to
7a1d04e
Compare
Addresses Issue #2906
Summary
Adds limit and pagination to the RSS feeds. The original issue requests an
afterparam, but I couldn't actually figure out the intent behind that nor did it seem to work with reddits rss feed. I'd be happy to address it if it's something we do want and someone can explain what it is, but for now substituting inpageas that seems more common and useful.Testing
Example query:
http://localhost:8536/feeds/c/test.xml?limit=1&page=2If there's 20 posts and you set the
limitto 10 andpageto 1, it will return the first 10 results. Setpageto 1 and it will return the next 10.