-
Notifications
You must be signed in to change notification settings - Fork 78
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
Tana to Obsidian importer #333
base: master
Are you sure you want to change the base?
Conversation
Hi @yole, I gave it a try, but couldn't get it to let me in without giving it credit card information :) |
|
||
const rootNode = this.tanaDatabase.docs.find(n => n.props.name && n.props.name.startsWith('Root node for')); | ||
if (!rootNode) { | ||
this.fatalError = 'Root node not found'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend throwing an exception and catching it in tana-json.ts
instead of storing in a variable.
|
||
const workspaceNode = this.nodes.get(rootNode.children[0]); | ||
if (!workspaceNode) { | ||
this.fatalError = 'Workspace node not found'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Root node not found", "Workspace node not found", etc. sound like errors that might not be meaningful to the average user of this plugin. Maybe this should just be logged to console.error
and the exception message should be something more generic such as Selected file ${filename} is not a valid Tana export.
this.markSeen(specialNode); | ||
} | ||
else { | ||
this.notices.push('Special node ' + suffix + ' not found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error also probably will not be actionable for most users.
private nodes: Map<string, TanaDoc>; | ||
private convertedNodes: Set<string> = new Set(); | ||
public fatalError: string | null; | ||
public notices: string[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this being consumed?
if (path) { | ||
this.notices.push('Found unconverted node: ' + path); | ||
unconverted++; | ||
if (unconverted == 50) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the significancy of 50 unconverted notes? Why are we breaking here?
if (unconverted == 50) break; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, I would suggest clearing the class-level variables which are only relevant to this call to importTanaGraph
. However, I think it would be even better to adjust this function to instead take the ProgressReporter
and to convert and save each file to the vault and report progress as it goes along and to not leavea any state behind on the class after being called. Take a look at some of the other format importers to see this pattern.
const properties: Array<[string, string, string]> = []; | ||
this.enumerateChildren(node, (child) => { | ||
if (child.props._docType == 'tuple' && child.children.length >= 2) { | ||
const propNode = this.nodes.get(child.children[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I reading correctly that properties for a node are stored as separate TanaDoc
instances?
} | ||
|
||
private convertNodeRecursive(node: TanaDoc, fragments: string[], indent: number) { | ||
if (node.props._docType == 'journal') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this is needed here?
} | ||
this.markAssociatedNodesSeen(node); | ||
if (indent == 0 && props.tag) { | ||
fragments.push('#' + props.tag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should go in the tags: []
frontmatter.
} | ||
|
||
private convertMarkup(text: string): string { | ||
return text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this all of the markup allowed in Tana?
Closes #327
I haven't received any feedback on the importer so far, so I'm submitting a PR as is. If this is merged, I'd be happy to address future issues as they arise.