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

Refactor sqlite #16

Merged
merged 13 commits into from
Feb 2, 2022
Merged

Refactor sqlite #16

merged 13 commits into from
Feb 2, 2022

Conversation

benetherington
Copy link
Owner

@fonji, can you take a look at this before I do any more work? There's some optimization left to do at the top of the file, but I'm mostly worried about the actual DB interfaces that get exported. Everything looks about as clean as it can get to me, but I'd like your opinion.

@benetherington
Copy link
Owner Author

benetherington commented Feb 2, 2022

Sorry to push commits into an active review! I promise I'm walking away for a bit now. :)

@fonji
Copy link
Collaborator

fonji commented Feb 2, 2022

Don't ever be sorry about pushing commits.
Just because I put myself as a reviewer does not mean I'm doing it right now anyway :)

Copy link
Collaborator

@fonji fonji left a comment

Choose a reason for hiding this comment

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

BOY that's WAY better!
Also, that's the kind of add/removed ratio I like
image

Two things

  • gotta get those conflicts solved
  • please reassure me on input sanitization

Comment on lines +161 to +162
"VALUES (?, ?, ?, ?);",
episode.episodeId, selectedAuthor.authorId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the ? trick enough to sanitize user inputs?
As the discord members like silliness, and XKCD, we don't want to run into a "little Bobby tables" suggestion

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes.

Though, A) I'm trusting this ticket, B) I absolutely need to include a Little Bobby Tables Easter egg into this thing at some point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool! I think you can trust it.

@benetherington benetherington merged commit 4418e7f into main Feb 2, 2022
@benetherington benetherington deleted the refactor-sqlite branch February 2, 2022 23:05
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