Skip to content
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

Adding a post_view mode. Fixes #3730 #3731

Merged
merged 13 commits into from
Aug 31, 2023
Merged

Adding a post_view mode. Fixes #3730 #3731

merged 13 commits into from
Aug 31, 2023

Conversation

dessalines
Copy link
Member

No description provided.

@@ -2,23 +2,6 @@ diff --git a/crates/db_schema/src/schema.rs b/crates/db_schema/src/schema.rs
index 255c6422..f2ccf5e2 100644
--- a/crates/db_schema/src/schema.rs
+++ b/crates/db_schema/src/schema.rs
@@ -2,16 +2,12 @@
Copy link
Member Author

@dessalines dessalines Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to remove this, because this PR adds the PostViewModeEnum in some conflicting lines.

This patch still retains the use diesel_ltree below, and it did work locally.

@@ -0,0 +1,3 @@
create type post_view_mode_enum as enum ('List', 'Card', 'SmallCard');

alter table local_user add column post_view_mode post_view_mode_enum default 'List' not null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether this should be called default_post_view_mode, or just post_view_mode

@Nutomic
Copy link
Member

Nutomic commented Jul 27, 2023

post_listing_mode seems like a better name.

@Nutomic Nutomic force-pushed the add_post_view_mode branch from b453b51 to e351d49 Compare August 9, 2023 10:10
@Nutomic Nutomic enabled auto-merge (squash) August 9, 2023 10:10
@@ -94,9 +96,10 @@ pub fn is_top_mod(
.map(|cm| cm.moderator.id)
.unwrap_or(PersonId(0))
{
Err(LemmyErrorType::NotTopMod)?;
Copy link
Member Author

@dessalines dessalines Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nutomic I found a bunch of these, that clippy fixed earlier, that could be scary, and I have no idea why rust isn't catching them.

Maybe I'm being paranoid, but it seems like the ; means that it wouldn't actually return the Err, even if it hit an Error result, and just continue on to the Ok(()) condition. If that's the case clippy should not be ignoring Err 's with a ;

This is why default Ok returns are really scary to me.

I went through all the ones I could find, and removed the semicolons, and removed the default returns where it was easy to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ? does an early return so the ; doesn't matter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, then rust is doing a good job of keeping us safe there.

@@ -29,7 +29,7 @@ pub async fn add_mod_to_community(
is_mod_or_admin(&mut context.pool(), local_user_view.person.id, community_id).await?;
let community = Community::read(&mut context.pool(), community_id).await?;
if local_user_view.local_user.admin && !community.local {
return Err(LemmyErrorType::NotAModerator)?;
Err(LemmyErrorType::NotAModerator)?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finished going through the codebase, and removed most of the return Err ... ; and changed them to Err ... (no semicolon) where possible. If you'd like, I can move that to a different PR.

Copy link
Member

@Nutomic Nutomic Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of code will be added right back if we dont prevent it via clippy. Closest I could find in this regard is try_err which only allows the way with return. To be honest that also looks clearer to me so I would prefer to keep the returns and add the lint.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. In that case I'll revert those last commits, and do a new PR to add try_err .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think I change my mind on this one. According to this lint, and this lint omitting the return is considered more idiomatic rust.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also ran the try_err --fix just to test it, and it conflicts with the needless_return lint, which then throws a ton of warnings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this as is then, since I did a lot of manual work to make those returns more logical.

@dessalines
Copy link
Member Author

All the api_tests seem to be passing locally, can't figure out why this keeps failing CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants