Skip to content

Working version with Pelton#8

Open
guptbot wants to merge 3 commits into
ms705:peltonfrom
guptbot:master
Open

Working version with Pelton#8
guptbot wants to merge 3 commits into
ms705:peltonfrom
guptbot:master

Conversation

@guptbot
Copy link
Copy Markdown

@guptbot guptbot commented Jun 13, 2022

No description provided.

Copy link
Copy Markdown
Owner

@ms705 ms705 left a comment

Choose a reason for hiding this comment

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

This looks good, but I left some comments for tweaks and a few questions.

What was the DateTime-related issue with this code? What you have seems entirely reasonable to me, but I recall you mentioned there is still an issue.

Comment thread src/admin.rs
let mut bg = backend.lock().unwrap();
let res = bg.prep_exec(
"SELECT * FROM questions WHERE lec = ?",
"SELECT lec , q , question FROM questions WHERE lec = ?",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why was this change necessary? Does Pelton not handle * correctly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nothing to do with pelton. This is because I added the extra primary key column to questions, and we dont want to be selecting that.

Comment thread src/backend.rs Outdated
Comment on lines +80 to +82
let mut insert_vals = String::new();
let temp = vals.iter().map(|_| "?").collect::<Vec<&str>>().join(",");
insert_vals += &temp;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Seems like it just creates an empty string, concatenates the question marks using the same code removed from below and then appends to the empty string. Why is it needed -- the resulting string seems to be the same as the one that the code you removed below would have generated?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I originally made these changes as I modified this method to make all necessary changes to inserts. I later changed that as you told me to but accidentally left this as is. Changing it now!

Comment thread src/backend.rs Outdated

pub fn replace(&mut self, table: &str, vals: Vec<Value>) {
self.do_insert(table, vals, true);
self.do_insert(table, vals, true);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

spurious indentation; either remove or run cargo fmt to auto-format your Rust code.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed!

Comment thread src/questions.rs Outdated
} else {
None
},
time: NaiveDateTime::parse_from_str(&String::from_utf8(from_value::<Vec<u8>>(r[4].clone())).unwrap(), "%Y-%m-%d %H:%M:%S").ok()
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As I now store the date as a string in the table, I made this edit to change that string to a NaiveDateTime as required by the rest of the code. The .ok() method at the end of this line returns an None if parse_from_string errors. And I believe that if we get None, the rest of the code decides to ignore the Date values, which is what seems to be happening as Dates are not getting printed in the frontend where they are supposed to.
I wan't able to find the issue with this line, its probably some silly mistake but it continues to elude me.

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