-
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
Config revamp #82
Config revamp #82
Conversation
See the README changelog for more context. In addition, note that the code that is related to exporting notion objects to files is now placed in `export.py`. Previously it had been shared between `notion.py`, `page.py`, `main.py`, and `database.py`. This refactor simplifies the `Client` class and greatly reduces the number of configuration items we need to pass into it. It also makes the separation of concerns cleaner.
This solution is simple, although it may have perforamance issues. Note that the `importlib.import_module` uses the internal python module cache.
I'm not sure how the tests are passing
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.
Awesome work @johndgiese! Overall, it looks good to merge to me. Just some nit picks.
@@ -582,8 +582,13 @@ def __init__(self, client, notion_data, page, get_children=True): | |||
def to_pandoc(self): | |||
# TODO: in the future, if we are exporting the linked page too, then add | |||
# a link to the page. For now, we just display the text of the page. | |||
if self.link_type == "page_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.
Is there a reason we're inlining this logic?
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 thought that, since we know what type of link it is, it would be better to avoid the unnecessary API request that's present if we use get_page_or_database
.
try: | ||
config = json.loads(config_json) | ||
except json.JSONDecodeError as exc: | ||
logger.error("Error parsing the data config JSON: %s", exc.msg) | ||
with open(path, "r") as config_file: | ||
config = yaml.safe_load(config_file) | ||
except yaml.YAMLError as exc: | ||
logger.error("Error parsing the config file: %s", exc) | ||
return None | ||
except FileNotFoundError: | ||
logger.error("The config file '%s' does not exist", path) | ||
return None | ||
if not validate_database_config(config): | ||
if not validate_config(config): | ||
logger.error("Invalid config file: %s", path) | ||
return None |
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.
Maybe we can extract this try/except out?
def load_config(path):
config = load_config_yaml(config_file)
if config is None:
return None
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 idea! Done!
master_defaults_copy = copy.deepcopy(builtin_defaults) | ||
defaults_copy = copy.deepcopy(defaults) | ||
config_item_copy = copy.deepcopy(config_item) | ||
merged_config_item = {**master_defaults_copy, **defaults_copy, **config_item_copy} |
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.
Didn't know you could update dictionaries this way. Cool!
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 sure you looked into this but I assume that with this syntax, the dictionaries get updated in the order they appear right? So it's equivalent to:
merged_config_item = master_defaults_copy
merged_config_item = merged_config_item.update(defaults_copy)
merged_config_item = merged_config_item.update(config_item_copy)
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.
That's right, the updates apply from left to right!
logger.error("Export config item missing the 'id' key") | ||
return False | ||
if not _valid_id(config_item["id"]): | ||
logger.error("Invalid id in export config item: %s", config_item["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.
Do we need a return False 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.
Yes! good catch!
counts['duplicate'] += 1 | ||
client = notion.Client(access_token, config["media_root"], config["media_url"]) | ||
|
||
for export in config['exports']: |
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 we extract out the stuff inside this for loop for readibility?
Perhaps:
for export in config['exports']:
client.load_plugins(export['plugins'])
export_node_from_config(export)
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.
Done!
Maybe we can also add an example config yaml file to the repository? |
- Provide a link back to the database that you don't have permission to - Suggest switching nested databases into a simple table
Note that the UUIDs are fake
FYI I didn't rebase because there were conflicts in the intermediate commits but not in the merge commit and it didn't seem worth sorting them out. |
@bimbashrestha this PR is ready for your review. I suggest reviewing commit-by-commit.