Generate SQL schema definition from CatColab schema #843
Conversation
quffaro
force-pushed
the
cm/sql-analysis
branch
from
04b248d to
ce5cea8
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
ce5cea8 to
d7642d1
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
61310f6 to
00cec35
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
00cec35 to
e68d9ff
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
0950224 to
6c01280
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
d90e26b to
8b46726
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
8b46726 to
9d3f076
Compare
This comment was marked as outdated.
This comment was marked as outdated.
jmoggr
force-pushed
the
cm/sql-analysis
branch
from
6764158 to
e189a4c
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
1ef0344 to
8189851
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
8189851 to
96b90f6
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
96b90f6 to
62a421b
Compare
quffaro
force-pushed
the
cm/sql-analysis
branch
from
62a421b to
2a71f77
Compare
| /// Tries to returns the morphism namespace | ||
| pub fn mor_namespace(&self) -> Result<Namespace, String> { | ||
| Ok(self.mor_namespace.clone()) | ||
| } |
There was a problem hiding this comment.
A couple issues with these methods: (1) they shouldn't return a Result because they are infallible and (2) they shouldn't clone because they should just be used for lookup, which doesn't require ownership.
In fact, you should be able to avoid exposing this data at all; see below.
| use sqlformat::{Dialect, format}; | ||
| use std::fmt; | ||
|
|
||
| impl Namespace { |
There was a problem hiding this comment.
Remove this impl. (Should be a code smell to be adding this here.)
|
|
||
| impl SQLAnalysis { | ||
| /// Constructs a new SQLAnalysis instance | ||
| pub fn new(obns: Namespace, morns: Namespace, backend: &str) -> Result<SQLAnalysis, String> { |
There was a problem hiding this comment.
Replace parameter obns: Namespace with
ob_label: impl Fn(&QualifiedName) -> QualifiedLabeland similarly for morphism labels.
|
|
||
| export function downloadTextContent(text: string, filename: string) { | ||
| return download(text, filename, "text/plain"); | ||
| } |
There was a problem hiding this comment.
Why export this function? It's only used once anyway; would suggest just deleting it.
| use crate::{ | ||
| one::Path, | ||
| zero::{Namespace, name}, | ||
| }; |
There was a problem hiding this comment.
I wish rustfmt did it automatically, but alas: these imports look messy, please clean them up a bit.
| .obns | ||
| .clone() | ||
| .label_name(model.get_cod(mor).unwrap_or(&name("")).clone()); | ||
| match format!("{}", tgt_name.clone()).as_str() { |
There was a problem hiding this comment.
I'd suggest moving this deeply nested match to a helper function for rendering attribute types.
There's also a certain amount of redundancy in the cases (ColumnDef::new(...)) that could likely be eliminated by refactoring the code slightly.
| use std::rc::Rc; | ||
| use tattle::Reporter; | ||
|
|
||
| impl DiscreteDblModel { |
There was a problem hiding this comment.
It's not your fault this feature wasn't properly exposed, but regardless this impl shouldn't be here. I'll see if I can improve that separately.
epatters
changed the title
No description provided.