Add support for generic timeline #525
No reviewers
Labels
No labels
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Plume/Plume#525
Loading…
Reference in a new issue
No description provided.
Delete branch "timeline"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
fix #450
Query example :
lang in [fr, en] and license in [cc] or followed or title contains "Plume"
todolist
make a gui to manage timelines for usermake a gui to manage timelines for instancemake a gui to manage lists for user&instanceadd tests for front end??Abstract syntax for query language
Codecov Report
blocked by #555
interface for managing timelines and lists will be made in a further pr
@ -0,0 +5,4 @@
use schema::{posts, timeline, timeline_definition};
use std::ops::Deref;
use {Connection, Error, PlumeRocket, Result};
What is this field for? Is it important to always have a defined order for timelines? And how should it be determined: by the user, or automatically?
@ -0,0 +5,4 @@
use schema::{posts, timeline, timeline_definition};
use std::ops::Deref;
use {Connection, Error, PlumeRocket, Result};
hum, I don't know, my through was that the first one would be on top, the second under and so on, but it clearly limit what can be done, I'll revert that
*snort
@ -0,0 +268,4 @@
let tl2_u1 = Timeline::new_for_user(
conn,
users[0].id,
"another timeline".to_owned(),
haah
@ -0,0 +268,4 @@
let tl2_u1 = Timeline::new_for_user(
conn,
users[0].id,
"another timeline".to_owned(),
I'm not good at this whole "giving name to lists" thing
@ -0,0 +268,4 @@
let tl2_u1 = Timeline::new_for_user(
conn,
users[0].id,
"another timeline".to_owned(),
i disagree, i think you're very good 😸
I think this PR is now complete. If you are fine with my commits, I think we can merge it?
there seems to be some failure when running migrations
at first glance this looks good. I'll run some tests when I get the time, to make sure all is ok
@ -0,0 +23,4 @@
)))
}
// TODO
will it be done in this pr, or is this just basic support and actual customization will be added later?
@ -0,0 +23,4 @@
)))
}
// TODO
I will add it later I think, this PR is big enough.
I think you are not calling the query parser when handling local like/reshare
I might be reading this wrong, but this enum (Kind) is made to tell if this is a new post, or a like/reshare of an "old" one, and by whom. This is in the part that handle likes, so I think this should be
Kind::Like(&actor)
. There is probably the same issue with reshare in other placessee previous comment
Indeed, thanks. I probably copied it from somewhere else and forgot to change it (I should really stop copy/pasting code…)
Thanks for the review, everything is fixed now.
But… I think posts are correctly removed from timelines when deleted, but not when un-liked or un-boosted (but this can probably be fixed later?).
they are remove because of cascading remove from sql. The easiest way to have the same comportment might be to add a reference to the like/reshare in 2 optional (and useless) columns, that cascade remove too
I can't approve this myself as it's my pr, so if someone could review it that would be nice
we should also do a little todolist of things yet missing.
What come to mind is :
@ -0,0 +1,51 @@
#![allow(dead_code)]
?
@ -0,0 +1,51 @@
#![allow(dead_code)]
It is because of the unimplemented routes, to avoid a few compilation warnings.
I seems OK for me (excepted for the things you listed), but I worked on it too, maybe you want to review it, @igalic ? (don't feel forced to do it, I think we can safely merge it if someone else approves the few commits I pushed).
And for the issue you listed, maybe it could be handled in another PR ? The basics are here, and this branch is already pretty big…
I'll see what i can do tonight.
@ -0,0 +1,51 @@
#![allow(dead_code)]
There is not warning about them, it's actually... dead code badoum tss
(no warning because functions are public, so the are maybe just used in a crate that depends on Plume, aka none)
What I listed was just to have a small todolist of things to add later, not now. This should end in an issue to not get forgotten
weak 👀 very tired, not finished this review sorry
why is this mutable?
can't we do:
same as above.
👍
Invalid
without thee
i dunno, this feels like an error…
@ -0,0 +23,4 @@
0 => Ok(ListType::User),
1 => Ok(ListType::Blog),
2 => Ok(ListType::Word),
3 => Ok(ListType::Prefix),
it would be nice if we had the same enum in SQL… but i digress.
there are some errors in your code (
i..
instead of0..
, ok() instead of unwrap...) but overall using for seems more logic than loopslightly less tired, more comments, would love to hear your feedback
👍
@ -0,0 +1,4 @@
-- This file should undo anything in `up.sql`
DELETE FROM timeline_definition WHERE name = 'Your feed';
DELETE FROM timeline_definition WHERE name = 'Local feed' AND query = 'local';
DELETE FROM timeline_definition WHERE name = 'Federared feed' AND query = 'all';
how does this work with translations?
i'm confused… why can't we just say
.first(&*c.conn)?;
mumbles something about macros
what does this do?
if we're searching by name, why do we need the user_id (i understand it's
Option
al, but still)@ -0,0 +284,4 @@
.select(list_elems::word)
.load::<Option<String>>(conn)
.map_err(Error::from)
.map(|r| r.into_iter().filter_map(|o| o).collect::<Vec<String>>())
i'm confused about the
filter_map()
in these functions.can you explain what it does?
is this still needed, or was this just for debugging?
@ -0,0 +53,4 @@
.map_err(Error::from)
} else {
timeline_definition::table
.filter(timeline_definition::user_id.is_null())
this is the only difference between the two expressions. I find it odd that there's no sensible, generic way to solve this.
@ -0,0 +68,4 @@
.map_err(Error::from)
} else {
timeline_definition::table
.filter(timeline_definition::user_id.is_null())
again, this duplication begs to be solved generically
if you turn the logic in this
if
around, we get one indentation level less.turn logic in
if
around to break out on top, and get one indentation level less.@ -0,0 +121,4 @@
*$state = Some(Token::Word($i,1,&""));
vec![]
},
_ => unreachable!(),
how informative is this
unreachable!()
actually gonna be, if we do ever hit it?@ -0,0 +151,4 @@
/// Private internals of TimelineQuery
#[derive(Debug, Clone, PartialEq)]
enum TQ<'a> {
what does
TQ
mean?@ -0,0 +490,4 @@
fn parse_d<'a, 'b>(mut stream: &'b [Token<'a>]) -> QueryResult<(&'b [Token<'a>], Arg<'a>)> {
match stream.get(0).map(Token::get_text)? {
s @ "blog" | s @ "author" | s @ "license" | s @ "tags" | s @ "lang" => {
what does this mean?
yay for refs
@ -0,0 +27,4 @@
#[get("/timeline/new")]
pub fn new() -> Result<Ructe, ErrorPage> {
unimplemented!()
what are these supposed to do ?
@ -111,0 +113,4 @@
"Your feed" => i18n!(cat, "Your feed"),
"Local feed" => i18n!(cat, "Local feed"),
"Federated feed" => i18n!(cat, "Federated feed"),
n => n.to_string(),
neato…
also, i just realized what this does, thanks to this awful example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ba47f87eb9d03239e40287e8c405e4ae
i'm quite happy with how the repetition of these ⬆️ in the templates you deleted is gone
i think we'll need
dir="auto"
here@ -0,0 +9,4 @@
@:base(ctx, tl.name.clone(), {}, {}, {
<section class="flex wrap" dir="auto">
<h1 class="grow">@i18n_timeline_name(ctx.1, &tl.name)</h1>
dir="auto"
?@ -0,0 +1,4 @@
-- This file should undo anything in `up.sql`
DELETE FROM timeline_definition WHERE name = 'Your feed';
DELETE FROM timeline_definition WHERE name = 'Local feed' AND query = 'local';
DELETE FROM timeline_definition WHERE name = 'Federared feed' AND query = 'all';
i think i found the answer to this somewhere below in the code!
First return a
Result<Blog, _>
, optional convert it to aResult<Option<Blog>,...>
so that if the error is not found, we can fetch the blog instead of not knowing it ever, but if the error is anything else, it's still an error@ -0,0 +284,4 @@
.select(list_elems::word)
.load::<Option<String>>(conn)
.map_err(Error::from)
.map(|r| r.into_iter().filter_map(|o| o).collect::<Vec<String>>())
It map each
Option<String>
to it's innerString
, and filterNone
s awaythis gives this module more visibility from the outer code, while keeping it private from other crates. It was probably necessary at some point, but rn it could be plain private
@ -0,0 +27,4 @@
#[get("/timeline/new")]
pub fn new() -> Result<Ructe, ErrorPage> {
unimplemented!()
allow creation/edition/deletion of custom timelines
@ -0,0 +121,4 @@
*$state = Some(Token::Word($i,1,&""));
vec![]
},
_ => unreachable!(),
I could probably rewrite that without unreachable! if it's needed (it's truly unreachable atm)
@ -0,0 +151,4 @@
/// Private internals of TimelineQuery
#[derive(Debug, Clone, PartialEq)]
enum TQ<'a> {
TimelineQuery, but more hidden (it's private while
TimelineQuery
is public)@ -0,0 +490,4 @@
fn parse_d<'a, 'b>(mut stream: &'b [Token<'a>]) -> QueryResult<(&'b [Token<'a>], Arg<'a>)> {
match stream.get(0).map(Token::get_text)? {
s @ "blog" | s @ "author" | s @ "license" | s @ "tags" | s @ "lang" => {
if it's equal to "blog", "author", "license"... store the value in s and match the given arm
I have no idea, @BaptisteGelez ?
I's a find by (list) name, in context of the given user (or instance if None)
@ -0,0 +53,4 @@
.map_err(Error::from)
} else {
timeline_definition::table
.filter(timeline_definition::user_id.is_null())
Welcome to the party!
Diesel consider that anything eq None unless we are that explicit
that needs either documentation or different names, because this way it's just way too confusing
thank you for the explanation!
@ -0,0 +151,4 @@
/// Private internals of TimelineQuery
#[derive(Debug, Clone, PartialEq)]
enum TQ<'a> {
perhaps add a comment such as:
or something along the lines
@ -0,0 +490,4 @@
fn parse_d<'a, 'b>(mut stream: &'b [Token<'a>]) -> QueryResult<(&'b [Token<'a>], Arg<'a>)> {
match stream.get(0).map(Token::get_text)? {
s @ "blog" | s @ "author" | s @ "license" | s @ "tags" | s @ "lang" => {
i have never seen that before! where is that documented?
@ -0,0 +27,4 @@
#[get("/timeline/new")]
pub fn new() -> Result<Ructe, ErrorPage> {
unimplemented!()
I the user interface? for which we have no design concept yet?
or via APIs?
@ -0,0 +27,4 @@
#[get("/timeline/new")]
pub fn new() -> Result<Ructe, ErrorPage> {
unimplemented!()
for the user interface. APIs are in their own directory
@ -0,0 +151,4 @@
/// Private internals of TimelineQuery
#[derive(Debug, Clone, PartialEq)]
enum TQ<'a> {
I think we should
#![warn(missing_docs)]
and progressively document everything in general@ -0,0 +490,4 @@
fn parse_d<'a, 'b>(mut stream: &'b [Token<'a>]) -> QueryResult<(&'b [Token<'a>], Arg<'a>)> {
match stream.get(0).map(Token::get_text)? {
s @ "blog" | s @ "author" | s @ "license" | s @ "tags" | s @ "lang" => {
https://doc.rust-lang.org/1.5.0/book/patterns.html#bindings
It's not useful very often, but when it is, it's very nice to have
@ -0,0 +53,4 @@
.map_err(Error::from)
} else {
timeline_definition::table
.filter(timeline_definition::user_id.is_null())
I've tried for a few hours to add a
eq_optional
via some trait, but the best I got was "only" 4 compiler error, of which the shortest was 4 lines long. I'm giving up on this@ -0,0 +53,4 @@
.map_err(Error::from)
} else {
timeline_definition::table
.filter(timeline_definition::user_id.is_null())
kudos for trying.
and I failed my merge >_<
👍
@ -0,0 +119,4 @@
}
})
{
Err(err)?;
That is 21 lines of error handling
@ -0,0 +157,4 @@
}
})
{
Err(err)?;
that's another 21 lines of error handling
there should be a better way to do that …
(I'm not saying you have to find that now)
@ -0,0 +724,4 @@
/*
#[test]
fn test_matches_lists_saved() {
let r = &rockets();
why is this one commented out?
@ -0,0 +805,4 @@
conn,
users[0].id,
"Linux subtitle".to_owned(),
"subtitle contains Stallman".to_owned(),
(i'd rather it didn't… lolsob…)
@ -0,0 +724,4 @@
/*
#[test]
fn test_matches_lists_saved() {
let r = &rockets();
I have no idea
@ -0,0 +805,4 @@
conn,
users[0].id,
"Linux subtitle".to_owned(),
"subtitle contains Stallman".to_owned(),
shall I edit that?
@ -0,0 +805,4 @@
conn,
users[0].id,
"Linux subtitle".to_owned(),
"subtitle contains Stallman".to_owned(),
We can do a cleanup PR and just remove all problematic techmen from our test examples
LGTM, this great PR have been waiting for too long!