Deferrable foreign keys for PostgresSQL backend#1057
Conversation
quffaro
added
enhancement
quffaro
force-pushed
the
cm/sql-improvement
branch
4 times, most recently
from
dd4e2ca to
0d06d77
Compare
| flex-direction: row; | ||
| } | ||
|
|
||
| .headerContainer > * { |
There was a problem hiding this comment.
I don't think this is necessary
quffaro
force-pushed
the
cm/sql-improvement
branch
3 times, most recently
from
ffe31d8 to
06efcb8
Compare
epatters
left a comment
epatters
left a comment
There was a problem hiding this comment.
Thanks Matt! I left a first round of questions and comments on the toposort alg.
I see lots of remaining TODOs, so maybe this wasn't intended to be reviewed yet?
| pub struct ToposortData<G> | ||
| where | ||
| G: FinGraph, | ||
| G::V: Hash + std::fmt::Debug + Clone, |
There was a problem hiding this comment.
It's considered bad practice to put trait bounds on the struct itself. Put them only on the impls. See https://stackoverflow.com/a/66369912
Moreover, you should make the type parameter be V (vertices) rather than G (graph).
| type ToposortResult<G> = Result<ToposortData<G>, ToposortError<G>>; | ||
|
|
||
| /// Implementation of topological sort which throws an error when it encounters a cycle. | ||
| pub fn toposort_strict<G>(graph: &G) -> ToposortResult<G> |
There was a problem hiding this comment.
I find this docstring confusing: in Rust, errors aren't thrown, exactly. Does this mean that an Err variant is returned when a cycle is encountered?
| } | ||
|
|
||
| /// Implementation of topological sort which does not throw an error when it encounters cycle. | ||
| pub fn toposort_lenient<G>(graph: &G) -> ToposortResult<G> |
There was a problem hiding this comment.
Does this mean that it always returns the Ok variant?
quffaro
force-pushed
the
cm/sql-improvement
branch
from
60b06a5 to
81e35cd
Compare
quffaro
force-pushed
the
cm/sql-improvement
branch
7 times, most recently
from
e3cf2cd to
d189b57
Compare
kasbah
left a comment
kasbah
left a comment
There was a problem hiding this comment.
Left some comments about syntax highlighting.
| import styles from "../styles.module.css"; | ||
| // eslint-disable-next-line import/no-unassigned-import | ||
| import "prismjs/components/prism-sql"; | ||
| // eslint-disable-next-line import/no-unassigned-import | ||
| import "prismjs/themes/prism.min.css"; | ||
|
|
There was a problem hiding this comment.
It would be nice to add have CodeView component so that the next person can just import and use that without worrying about global imports.
It seems possible, if a little tricky, to support all languages but lazily loaded. Maybe PrismJS isn't the best to do this in a clean way. My first thought was something tree-sitter based but a quick search didn't turn up any obvious solution.
There was a problem hiding this comment.
I'm willing to work on that in a separate PR.
There was a problem hiding this comment.
Still for this PR what is currently here could go in a CodeView component and it only supports SQL.
quffaro
force-pushed
the
cm/sql-improvement
branch
4 times, most recently
from
d03a64a to
fdd4db3
Compare
quffaro
force-pushed
the
cm/sql-improvement
branch
2 times, most recently
from
2fcba2f to
9f07cba
Compare
quffaro
force-pushed
the
cm/sql-improvement
branch
from
9f07cba to
59b5a56
Compare
quffaro
force-pushed
the
cm/sql-improvement
branch
3 times, most recently
from
a52b6fa to
c7446d0
Compare
quffaro
force-pushed
the
cm/sql-improvement
branch
from
c7446d0 to
a70b2f2
Compare
3 participants
Closes #1030 and #1034