LemmyNet
/
lemmy
3
2
Fork 3
Code Pull Requests 1 Releases Wiki Activity

Remove dead code #81

Merged
dessalines merged 7 commits from remove-dead-code into main 2020-08-12 12:30:54 +00:00
Conversation 2 Commits 7 Files Changed 20 +22 -205
nutomic commented 2020-08-11 14:24:37 +00:00
Owner
Copy Link

There is just no reason to keep code around that we dont use. In the best case, it slows down compilation, and in the worst case, it introduces bugs because someone uses the wrong database function. So I removed all of it, based on the output of warnalyzer. If needed, we can always do git blame to recover that code.

Rust unit tests are failing for me on master, so I'm not sure if this breaks any tests.

Based on my test, this speeds up clean debug builds of the lemmy_server crate (excluding dependencies) from 20s to 15s. But I only tried once so this could be random fluctuation.

There is just no reason to keep code around that we dont use. In the best case, it slows down compilation, and in the worst case, it introduces bugs because someone uses the wrong database function. So I removed all of it, based on the output of [warnalyzer](https://github.com/est31/warnalyzer). If needed, we can always do git blame to recover that code. Rust unit tests are failing for me on master, so I'm not sure if this breaks any tests. Based on my test, this speeds up clean debug builds of the lemmy_server crate (excluding dependencies) from 20s to 15s. But I only tried once so this could be random fluctuation.
nutomic added a new dependency 2020-08-11 14:24:59 +00:00
#80 Remove usage of Option::unwrap() in activitypub code
dessalines commented 2020-08-11 14:36:06 +00:00
Owner
Copy Link

This needs a re-do after a merge from main. There are also some things I wanna keep because we'll use them shortly, I'll comment on them.

This needs a re-do after a merge from main. There are also some things I wanna keep because we'll use them shortly, I'll comment on them.
dessalines reviewed 2020-08-11 14:37:22 +00:00
server/lemmy_db/src/activity.rs
@ -36,4 +36,1 @@
fn delete(conn: &PgConnection, activity_id: i32) -> Result<usize, Error> {
use crate::schema::activity::dsl::*;
diesel::delete(activity.find(activity_id)).execute(conn)
dessalines commented 2020-08-11 14:37:22 +00:00
Owner
Copy Link

Ugh, I get that these are never used... but it seems like a bad idea to change them to unimplemented and different from all the others. I'm open to it I guess tho.

Ugh, I get that these are never used... but it seems like a bad idea to change them to unimplemented and different from all the others. I'm open to it I guess tho.
nutomic commented 2020-08-11 17:55:52 +00:00
Author
Owner
Copy Link

Or maybe we could add a Crud variant without the delete function?

Or maybe we could add a `Crud` variant without the delete function?
dessalines commented 2020-08-11 19:39:09 +00:00
Owner
Copy Link

You can just make that one optional.

[You can just make that one optional](https://stackoverflow.com/questions/51915551/how-does-one-define-optional-methods-on-traits).
dessalines commented 2020-08-11 19:48:38 +00:00
Owner
Copy Link

Oh and then you can remove all those unimplemented() ones.

Oh and then you can remove all those `unimplemented()` ones.
dessalines commented 2020-08-11 19:49:11 +00:00
Owner
Copy Link

Hrm its not showing the link well: https://stackoverflow.com/questions/51915551/how-does-one-define-optional-methods-on-traits

Hrm its not showing the link well: https://stackoverflow.com/questions/51915551/how-does-one-define-optional-methods-on-traits
nutomic commented 2020-08-12 11:41:32 +00:00
Author
Owner
Copy Link

Ah perfect!

Ah perfect!
dessalines reviewed 2020-08-11 14:38:26 +00:00
server/lemmy_db/src/comment.rs
@ -57,4 +57,1 @@
fn delete(conn: &PgConnection, comment_id: i32) -> Result<usize, Error> {
use crate::schema::comment::dsl::*;
diesel::delete(comment.find(comment_id)).execute(conn)
dessalines commented 2020-08-11 14:38:26 +00:00
Owner
Copy Link

Comment, post, user and community deletes shouldn't be removed, bc we will use them once we implement purge. (I think one of the chapo team was maybe working on it)

Comment, post, user and community deletes shouldn't be removed, bc we will use them once we implement purge. (I think one of the chapo team was maybe working on it)
nutomic commented 2020-08-11 17:51:49 +00:00
Author
Owner
Copy Link

Okay, restored them.

Okay, restored them.
dessalines marked this conversation as resolved
dessalines reviewed 2020-08-11 14:39:48 +00:00
server/lemmy_db/src/post.rs
@ -292,4 +257,0 @@
)
.execute(conn)
}
}
dessalines commented 2020-08-11 14:39:48 +00:00
Owner
Copy Link

This should all stay here, in case we add some functionality later on where your front page hides posts you've already read.

This should all stay here, in case we add some functionality later on where your front page hides posts you've already read.
nutomic commented 2020-08-11 17:54:02 +00:00
Author
Owner
Copy Link

I dont really like this thing of implementing things that might be needed in the future, because either it never gets used, or it needs to get changed anyway at that time. And in the meantime its just making things more complicated for no benefit at all. Would you be happy if I add a comment with a link to this PR, so that the previous code is easy to find?

I dont really like this thing of implementing things that might be needed in the future, because either it never gets used, or it needs to get changed anyway at that time. And in the meantime its just making things more complicated for no benefit at all. Would you be happy if I add a comment with a link to this PR, so that the previous code is easy to find?
dessalines commented 2020-08-11 19:42:08 +00:00
Owner
Copy Link

Ugh, even removing the passing unit tests? No consider this something we will implement. I'll open up a ticket for hiding read posts.

Ugh, even removing the passing unit tests? No consider this something we will implement. I'll open up a ticket for hiding read posts.
nutomic marked this conversation as resolved
dessalines reviewed 2020-08-11 14:41:09 +00:00
server/lemmy_utils/src/settings.rs
@ -123,4 +123,1 @@
pub fn api_endpoint(&self) -> String {
format!("{}/api/v1", self.hostname)
}
dessalines commented 2020-08-11 14:41:09 +00:00
Owner
Copy Link

Might be a good idea to use this, rather than the hardcoded v1 string in the api.... but I spose we can remove.

Might be a good idea to use this, rather than the hardcoded `v1` string in the api.... but I spose we can remove.
nutomic commented 2020-08-11 17:54:57 +00:00
Author
Owner
Copy Link

We will only need that once we get a v2 api, and by then it needs to be rewritten anyway. No need to make things more complicated now.

We will only need that once we get a v2 api, and by then it needs to be rewritten anyway. No need to make things more complicated now.
dessalines commented 2020-08-11 19:43:23 +00:00
Owner
Copy Link

Sounds good.

Sounds good.
dessalines marked this conversation as resolved
nutomic commented 2020-08-12 11:50:13 +00:00
Author
Owner
Copy Link

Okay I added post_read back in, made the delete function optional and removed Likeable::read (which was completely unused).

Please squash when you merge.

Okay I added post_read back in, made the delete function optional and removed Likeable::read (which was completely unused). Please squash when you merge.
dessalines merged commit aace1bd71e into main 2020-08-12 12:30:54 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Depends on
#80 Remove usage of Option::unwrap() in activitypub code
LemmyNet/lemmy
Reference: LemmyNet/lemmy#81
No description provided.
Delete Branch "remove-dead-code"

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?