Skip to content

Conversation

@Jm0rr
Copy link

@Jm0rr Jm0rr commented Nov 24, 2022

…of group if false and leave empty if true -- issue #12

Copy link
Owner

@quantumish quantumish left a comment

Choose a reason for hiding this comment

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

Left a few syntax-related pieces of feedback. Also, could you update any calls to this method so that they include this new change? Off the top of my head I think it should just be the call in tests.rs, but probably worth double checking so that things don't crash.

self.db.add_group_channel(gid.0, cid).unwrap();
channel = db.get_channel(cid).unwrap();
if private { // making the channel private
make_channel_private(channel)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this needs a semicolon in order to compile

if private { // making the channel private
make_channel_private(channel)
}
else // adding existing users to channel
Copy link
Owner

Choose a reason for hiding this comment

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

Missing a bracket?

else // adding existing users to channel
let users = self.db.get_group_users(gid.0).unwrap();
for user in users {
self.db.add_channel_member(channel, user).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

No close bracket here either

let fixed_name = check_name(name.0.clone());
self.db.create_channel(cid, gid.0, auth.0.id, fixed_name.clone()).unwrap();
self.db.add_group_channel(gid.0, cid).unwrap();
channel = db.get_channel(cid).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be let channel = self.db.get_channel(cid).unwrap();?

@Jm0rr
Copy link
Author

Jm0rr commented Nov 24, 2022

I just fixed the syntax errors (they were already on my local file, oops) and made it so the tests won't break. The new feature isn't being tested though, so I will write that next.

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.

2 participants