-
Notifications
You must be signed in to change notification settings - Fork 57
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
#204 - Evernote bug bounty #207
Conversation
@akosbalasko thanks! Will review shortly. Two more things:
|
@kepano I have read the CLA agreement and I hereby sign the CLA. |
doc PR is here: obsidianmd/obsidian-help#670 |
@@ -40,7 +41,8 @@ export const hasSourceURLInTemplate = (templateContent: string): boolean => { | |||
}; | |||
export const hasAnyTagsInTemplate = (templateContent: string): boolean => { | |||
return (hasItemInTemplate(TAGS, templateContent) | |||
|| hasItemInTemplate(YAMLARRAYTAGS, templateContent)); |
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.
Should we remove the various bits of code for array tags now? It looks like they are unused?
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.
well, yeah, I was thinking on it, as you wish, I think the whole array tags in yaml together with its templates is currently not used. @lishid any suggestions?
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 think removing it would be best, thanks!
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.
@lishid removed
@@ -77,9 +77,13 @@ const processResource = (workDir: string, resource: any): any => { | |||
// Skip unknown type as we don't know how to handle | |||
// Source: https://dev.evernote.com/doc/articles/data_structure.php | |||
// "The default type "application/octet-stream" should be used if a more specific type is not known." | |||
if (resource.mime === 'application/octet-stream') { | |||
// Update: |
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.
fs.mkdirSync(path.join(options.outputDir, ...notebookFolderNames), { recursive: true }); | ||
enex.fullpath = enex.fullpath.replace(enex.basename, notebookName || enex.basename); | ||
enex.name = enex.name.replace(enex.basename, notebookName || enex.basename); | ||
options.outputDir = [options.outputDir, ...notebookFolderNames].join(options.pathSeparator); |
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.
Will this behave as expected if there are multiple files being imported?
It looks like if we modify the outputDir here it will affect subsequent files being imported.
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.
Nope, because I save the original outputDir at line 187 in yarle.ts and restore after the conversion (line 201).
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 believe that's just saving a reference to the object, but that's the same object that will be modified here.
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.
@tgrosinger I think that would be true in the case if an object would have been stored in the variable, but in this case, it is just a copy of a primitive value. Here is an example:
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.
Ah, sorry I misread and thought you were storing the full options object.
Thank you for clarifying for me!
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.
While this is technically correct, it is really a bad design pattern because other parts of the code may not know or understand why the outputDir was changed. It gives the same variable two meanings at different times of the code execution that would be confusing.
In this case it's best if we can return a value and have it passed along to the next functions/
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.
@lishid ok, let me try to improve it.
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.
Looks good to me, pending the remaining comments for @lishid.
@@ -37,7 +37,10 @@ export const getResourceFileProperties = (workDir: string, resource: any): Resou | |||
|
|||
if (resource['resource-attributes'] && resource['resource-attributes']['file-name']) { | |||
const fileNamePrefix = resource['resource-attributes']['file-name'].substr(0, 50); | |||
fileName = fileNamePrefix.split('.')[0]; | |||
const lastIdx = fileNamePrefix.lastIndexOf('.'); |
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 suggest using the existing filename parser at https://github.com/obsidianmd/obsidian-importer/blob/master/src/filesystem.ts#L210
fs.mkdirSync(path.join(options.outputDir, ...notebookFolderNames), { recursive: true }); | ||
enex.fullpath = enex.fullpath.replace(enex.basename, notebookName || enex.basename); | ||
enex.name = enex.name.replace(enex.basename, notebookName || enex.basename); | ||
options.outputDir = [options.outputDir, ...notebookFolderNames].join(options.pathSeparator); |
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.
While this is technically correct, it is really a bad design pattern because other parts of the code may not know or understand why the outputDir was changed. It gives the same variable two meanings at different times of the code execution that would be confusing.
In this case it's best if we can return a value and have it passed along to the next functions/
notebookName = enex.basename; | ||
} | ||
fs.mkdirSync(path.join(options.outputDir, ...notebookFolderNames), { recursive: true }); | ||
enex.fullpath = enex.fullpath.replace(enex.basename, notebookName || enex.basename); |
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.
These aren't meant to be modified. PickedFile represents a file picked from the operating system. If we need to work on a different set of names over the original file then we should use a proper data structure to make this clear.
Also, this wouldn't work if the full path contains a piece that is identical to the enex basename. For example, C:/Evernote/Files/Evernote.enex
would only get the first "Evernote" replaced instead of the intended second.
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.
These aren't meant to be modified. PickedFile represents a file picked from the operating system. If we need to work on a different set of names over the original file then we should use a proper data structure to make this clear.
@lishid But in this case, it would mean just to clone the current PickedFile, changing its props and use it instead of that we got from outside, or not?
Also, this wouldn't work if the full path contains a piece that is identical to the enex basename. For example, C:/Evernote/Files/Evernote.enex would only get the first "Evernote" replaced instead of the intended second.
Good point I'll resolve it.
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.
@lishid But in this case, it would mean just to clone the current PickedFile, changing its props and use it instead of that we got from outside, or not?
Can you explain the use case here? If I'm understanding it correctly you'd like to specify a different title/output path for the same input file, right? In that case I would suggest passing this additional information over to the code that uses it (through an object or function parameters) to clearly indicate that it's the supposed output path.
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.
@lishid yeah, that would be the use case, and I used to use options object would to store these kind of info. Now I cleaned a code a bit around notebook stacks, hope it looks better for you too.
@@ -40,7 +41,8 @@ export const hasSourceURLInTemplate = (templateContent: string): boolean => { | |||
}; | |||
export const hasAnyTagsInTemplate = (templateContent: string): boolean => { | |||
return (hasItemInTemplate(TAGS, templateContent) | |||
|| hasItemInTemplate(YAMLARRAYTAGS, templateContent)); |
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 think removing it would be best, thanks!
Sorry I am late! |
const { notebookName } = getNotebookNameAndFolderNames(baseEnex.basename); | ||
|
||
const stackedPickedFile = new NodePickedFile((baseEnex as NodePickedFile).filepath); | ||
stackedPickedFile.fullpath = replaceLastOccurrenceInString(baseEnex.fullpath, baseEnex.basename, notebookName || baseEnex.basename); |
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.
Sorry I didn't make this clear; we should not be modifying internal data structures of NodePickedFile. I have made an update to the API on master, if you can rebase you will understand what I meant.
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.
@lishid Ohh, okay, now I get it, thanks for the clarification! Changes made to make it work with untouchable NodePickedFiles. :)
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.
As I understand it, what's happening here is that we are finding some code down the line that uses these fields, and we'd like to change their behavior to use a different names/paths. Doing it this way is a "shortcut"; while it results in minimal amount of changes, it also makes the code much more confusing. Similar to what I said in the other comment, when you have a field that is defined to mean one thing, and you use it for a slightly different purpose (and thus modify the contents to fit your alternative needs), it creates a chance of confusion, or at the very least, forces the reader to "keep in mind" these extra concerns. This is often where bugs can slip in, especially if there are many different little things one needed to keep in mind when working with the code.
Usually the best way to do this is being more explicit with the code to say what you intend with the variables and fields. Passing additional vars with names that indicate what they do (or is intended for) helps a lot.
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.
Yup the new changes works for me.
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.
@lishid , sorry, I had a bad feeling about this, so I tried to improve the code further, introduced a couple of new functions to decrease the level of the possible confusions. I hope you agree, but if not, feel free to ask to revert.
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.
Thanks. I would say don't worry too much about it. Yarle is unfortunately written in a very different paradigm of programming than the rest of the importer. It uses global singleton objects and naked functions with context being passed sometimes, set in the global object at other times.
I decided not to invest too much effort to rewrite yarle as it would not be worth the time for a functionally equivalent package.
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.
@lishid Okay, I see. Thanks for the review! Then we can move forward with it, right?
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.
Code looks ok to me. Might be worth doing one final pass of the auto formatter.
Looks good to me. |
Evernote improvements and fixes: #204
#190 - Fixed
#200 - Implemented
#26 - Fixed
#201 - Fixed
#24 - Implemented, "@@@" used as delimiter between notebookstack's and notebook's name in the enex file
#187 - Cannot be implemented phisically, possible solution would be to ask the users to flat nested tags by '/' before the conversion within Evernote
#194 - Cannot reproduce, possible reason patched
#184 - Cannot reproduce, reporter asked to close
#185 - Cannot reproduce, no hints from reporter
+1 - Title removed from the converted note's body