-
Notifications
You must be signed in to change notification settings - Fork 1
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
Basic working version #5
Conversation
(not all block types supported yet)
(external links only)
+ refactored converted into objects + added image parser + add command line args related to images
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.
Really nice work Ilya, the code is looking great! I played around with the Commandline tool a little bit. In addition to addressing my comments, if you could try pulling down this database:
https://www.notion.so/innolitics/01dee73fef7e4dd2a02a2a50b6848ba1?v=77043ea15048428ca095601714176e0f
That would be great. The access token is in 1password under the "Notion "Website" Integration Token" entry. The integration is already added to the page and has read-only access.
I think this is the most important comment: https://github.com/innolitics/n2y/pull/5/files#r799131270
I'd be happy to jump on a call to discuss tomorrow.
n2y/main.py
Outdated
" yaml - print yaml to stdout\n" | ||
" markdown - create a mardown file per page")) | ||
parser.add_argument("--image-path", help="Specify path where to save images") | ||
parser.add_argument("--image-web-path", help="web path for images") |
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.
Small detail, but the case of the help strings is inconsistent; some are capitalized and some aren't.
n2y/main.py
Outdated
if options.name_column not in raw_rows[0]: | ||
print(f"Database does not contain the column \"{options.name_column}\". " | ||
f"Please specify the correct name column using the --name-column flag.") | ||
exit(1) |
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.
Instead of calling exit
in these, I think it would be a little cleaner to return the exit code here and then again in main
, that way all of the exits go through this line:
This would let us test main
, for example, if we wanted to.
n2y/main.py
Outdated
return 0 | ||
|
||
|
||
def export_markdown(client, database_id, options): | ||
raw_rows = client.get_database(database_id) |
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.
It seems like there's bit of duplicated code at the top of export_markdown
and export_yaml
that could be avoided by pulling this code up a level.
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.
Duplicate code consolidated.
Co-authored-by: David Giese <johndgiese@gmail.com>
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.
Ready for round 2
n2y/converter.py
Outdated
|
||
|
||
def load_plugins(filename): | ||
global Bookmark, BulletedList, BulletedListItem, ChildPageBlock, CodeBlockFenced, \ |
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 agree that this is repeating a lot of names, but I'm not sure how this can be done in a better way. Open to suggestions.
n2y/converter.py
Outdated
plugin_spec = importlib.util.spec_from_file_location("plugins", abs_path) | ||
plugin_module = importlib.util.module_from_spec(plugin_spec) | ||
plugin_spec.loader.exec_module(plugin_module) | ||
print(plugin_module) |
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.
good catch
self.type = 'to_do_list' | ||
super().__init__(client, block, get_children) | ||
if self.checked: | ||
self.text.text[0].plain_text.text = '☒ ' + self.text.text[0].plain_text.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.
As far as I can tell, this is a pandoc quirk. See http://boisgera.github.io/pandoc/markdown/#extension-task_lists
Pandoc will output GitHub flavored checkboxes when it processes them.
self.url = obj['external']['url'] | ||
|
||
def download(self): | ||
# TODO: append created time as hex to end of file to prevent collisions? |
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'm not sure it's a good idea to change the URL of a web page every time the content changes. Another idea is to include Notion block id in the name. It's unique and constant. Date idea was floated because it would be shorter than Notion block id. Using the block id also has the advantage of being able to find the page in Notion from a markdown file.
- Make sure the title column is not empty - Skip pages with no names and show a warning - Warn about entries without a page - Warn about duplicate file names
Updated per comments. |
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 had a few small suggestions, but feel free to merge once they're addressed!
n2y/converter.py
Outdated
Toggle = value | ||
if key in globals(): | ||
# plugins can only override classes in this file that are derrived from a Block | ||
if globals()[key].__name__ in [b.__name__ for b in value.__bases__] \ |
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 this line can be simplified by using some intermediate variables (e.g., for globals()[key]
and [b.__name__ for b in value.__bases__]
.
return 0 | ||
first_row_flattened = simplify.flatten_database_row(raw_rows[0]) | ||
|
||
def available_columns(): |
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.
Why use a function for this instead of just calculating the value? Is this to avoid calculating the value if it doesn't end up being used?
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.
Yes, there are two places where the available columns may be shown, but there is usually no need to generate this list. Also, I imagine this function will grow in future versions as we would want deeper checks.
Notion Blocks supported:
Supported styling:
Blocks can be extended or replaced using a plugin system.
Addresses issues #1, #2, & #3.