Database by kerrhau · Pull Request #10 · strinking/markov-bot

kerrhau · 2017-07-03T12:54:51Z

No description provided.

Created a new generate_from_word function

emmiegit · 2017-07-03T22:53:53Z

+                }
+            }
+            None => {
+                ctx.set_presence(None, OnlineStatus::Online, false);


Lots of repeated code here. Change this to a function that returns an OnlineStatus, and then only call ctx.set_presence() once based on that.

emmiegit · 2017-07-03T22:54:47Z

+command!(name(ctx, message, args) {
+    if ALLOWED_USER_IDS.contains(&message.author.id.0) {
+        let arg = args.join(" ");
+        let _ = ctx.edit_profile(|p| p.username(arg.as_str())).expect("could not set name");


.expect() will panic, killing the program. Is this really what you want if you can't set your name?

emmiegit · 2017-07-03T22:57:00Z

+    let _ = ctx;
+    if ALLOWED_USER_IDS.contains(&message.author.id.0) {
+        let arg = args.join(" ");
+        let _ = message.guild_id().unwrap().edit_nickname(Some(arg.as_str()));


message will only have a guild_id() if it is from a guild. However, you perform no checks to make sure this is the case. If somebody sent this bot a DM with this command, it would crash.

emmiegit · 2017-07-03T22:58:48Z

+                None => {
+                    let _ = message.channel_id.say(ERROR_MESSAGE);
+                }
+            }


This match should return a &str, and that should be fed into .say()
Otherwise you're duplicating code.

emmiegit · 2017-07-03T22:58:59Z

+
+                None => {
+                    let _ = message.channel_id.say(ERROR_MESSAGE);
+                }


emmiegit · 2017-07-03T23:13:02Z

+
            None => {
-                // Message is empty
+                println!("Empty message");


Instead of println!() this should use some sort of proper logging.

emmiegit · 2017-07-03T23:13:34Z

+    pub fn generate_from_word(&self,
+                              length: u32,
+                              starting_word: Option<&String>)
+                              -> Option<String> {


wtf is this what rustfmt gave you?

emmiegit · 2017-07-03T23:14:10Z

+    pub content: String,
+    pub guild_id: i64,
+    pub author_id: i64,
+    pub channel_id: i64,


Discord IDs are always u64

postgres doesn't support u64

Then make the internal API support u64. Have only the code that directly glues the database to the rest of the program use i64.

emmiegit · 2017-07-03T23:14:31Z

+                              starting_word: Option<&String>)
+                              -> Option<String> {
+        let mut rng = thread_rng();
+        let mut word: &String;


wtf &String

Just do &str

emmiegit · 2017-07-03T23:15:39Z

+                    break;
+                }
+            }
+        }


This is just moved from the original generate, right? No changes?

emmiegit

Some new issues. There are a fair number of things you haven't fixed from my first review.

emmiegit · 2017-07-07T21:12:25Z

-command!(help(ctx, msg, args) {
+lazy_static! {
+    static ref ALLOWED_USER_IDS: Vec<u64> = vec![76043245804589056, 98633956773093376];
+}


I'll let it slide for this PR, but you should really make a proper configuration file.

emmiegit · 2017-07-07T21:13:14Z

+
+command!(status(ctx, message, args) {
+    if ALLOWED_USER_IDS.contains(&message.author.id.0) {
+        ctx.set_presence(None, Status::from_str(args.get(0).unwrap()).unwrap_or(OnlineStatus::Online), false);


Clean this line up. Like the part where you get the OnlineStatus should be its own line.

emmiegit · 2017-07-07T23:45:04Z

+        } else {
+            let _ = message.guild_id().unwrap().edit_nickname(Some(arg.as_str()));
+        }
+    }


Why do all of these only work only apply to "allowed users"?

If you're constantly doing membership checks, why not put them in a HashSet<T>?

emmiegit · 2017-07-07T23:46:09Z

+                }
+            },
+            None => {
+                length += DEFAULT_GENERATION_LENGTH;


It still isn't fixed...

emmiegit · 2017-07-07T23:47:01Z

+command!(generate_from_word(ctx, message, args) {
+    let mut data = ctx.data.lock().unwrap();
+    let mut markov = data.get_mut::<Markov>().unwrap();
+    let mut word: Option<&str> = None;


I don't see why word needs to be mutable.

it's written to later on

No I mean like, why not set it's value either within a function or within a {} scope
and then set it to a non-mut binding then?

emmiegit · 2017-07-07T23:51:03Z

+        markov::parse_messages(&connection.lock().unwrap(),
+                               data.get_mut::<Markov>().unwrap());
+        markov::parse_user_messages(&connection.lock().unwrap(),
+                                    data.get_mut::<UserMap>().unwrap());


This is going to take forever. Why can't you async this?

emmiegit · 2017-07-07T23:51:47Z

-            }
-        }
+        let message_id = msg.id.0;
+        let message_content: String = String::from(msg.content_safe());


Type annotation unneeded here.

emmiegit · 2017-07-07T23:52:12Z

-            }
-        }
+        let message_id = msg.id.0;
+        let message_content: String = String::from(msg.content_safe());


Also you call .content_safe() twice. Why not just reuse stripped?

emmiegit · 2017-07-07T23:52:37Z

+                                 channel_id);
+
+        let mut data = ctx.data.lock().unwrap();
+        let mut new = ctx.data.lock().unwrap();


uhh...

To ask the obvious, why is this being called twice?

emmiegit · 2017-07-07T23:53:10Z

+        markov.parse(&stripped);
+        usermap
+            .entry(author_id)
+            .or_insert(Markov::new())


Use a callback function to avoid creating unnecessary Markovs every time.

emmiegit · 2017-07-08T18:46:44Z

+        .map_or(DEFAULT_GENERATION_LENGTH,
+                |x| x.parse::<u32>()
+                .ok()
+                .unwrap_or(DEFAULT_GENERATION_LENGTH));


emmiegit · 2017-07-08T18:47:09Z

+        let locked_data1 = client.data.clone();
+        let locked_data2 = client.data.clone();
+        let locked_connection1 = connection.clone();
+        let locked_connection2 = connection.clone();


Why do you need two copies of data and connection?

emmiegit · 2017-07-08T18:47:58Z

+                .entry(author_id) 
+                .or_insert(Markov::new()) 
+                .parse(&stripped); 
+        }


👍 for using local { } scopes

emmiegit · 2017-07-08T18:48:14Z

+            let mut usermap = data.get_mut::<UserMap>().expect("UserMap does not exist"); 
+            usermap 
+                .entry(author_id) 
+                .or_insert(Markov::new()) 


Use a callback to avoid creating unnecessary Markov instances.

kerrhau and others added 18 commits June 24, 2017 07:51

Merge pull request #3 from strinking/markov-fix

8b63b8b

Started adding on generate by starting word, added url filtering

bbc87c0

Added generate by word

db7ce59

Fixed variable naming

489b948

Added needed dependency

2247b1e

Fixed help command

2d4db5e

Fixed help command

e255d34

Split generate into 2 different functions

18d3ca1

Created a new generate_from_word function

Added needed dependencies and files for the message database

ef62f5f

Added status change command

7269035

Fixed mention issue

3254982

Added game command

783cf1a

Added change game and name change commands

37aff63

Fixed the generate command by splitting normal generates and word gens

9fd0211

fixed gitignore

bb5b390

Merge branch 'latest' into database

3d70274

Added nickname change command

6966c92

Added database of messages (user messages included)

2c84e54

emmiegit suggested changes Jul 3, 2017

View reviewed changes

kerrhau added 8 commits July 5, 2017 19:26

Fixed repetitive match arm

2b9534b

Added status file

47c2ec8

Run rustfmt

7762b2f

Run rustfmt & small changes

3557b19

Fixed the generate_from_word function

2c6f17c

Fixed the generate by word function and added a bot is running notifier

3b3ae9c

Removed the need to cast u64 to i64 before calling insert on the db

5b25a4c

Run rustfmt

2969403

emmiegit suggested changes Jul 7, 2017

View reviewed changes

Added threading and added changes for efficiency

9afa63a

emmiegit suggested changes Jul 8, 2017

View reviewed changes

kerrhau and others added 3 commits July 9, 2017 00:44

Update README.md

0922cfa

Syntax changes

84598f5

Added changes to the parsing system and fixed repetitive code issues

f0f04bb

Uh oh!

Conversation

kerrhau commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmiegit Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmiegit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

emmiegit Jul 4, 2017 •

edited

Loading