Skip to content
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

update how database filters children and add version flag #89

Merged
merged 14 commits into from
Dec 5, 2022

Conversation

sHermanGriffiths
Copy link
Contributor

@sHermanGriffiths sHermanGriffiths commented Nov 23, 2022

Author's Checklist

Before assigning reviewers, step through the following checklist. Check each item once it's completed. If an item is skipped, write an explanation below the item.

  • Construction: Review the code diff and confirm there's no dead code, debug code, or unnecessary comments.
  • Requirements: Review the requirements in the issue for this PR and confirm they're all implemented.
  • Architecture: Confirm that the changes to the code are consistent with the existing architecture.
  • SOUP/OTS Software: Any new dependencies have appropriate licenses and are documented or pinned in requirements files.
    N/A
  • Unit Test: Atomic unit tests have been added, if appropriate.
    N/A
  • End-to-End Tests: End-to-end tests have been extended or updated, if appropriate.
    N/A
  • Low-level Documentation: Comments or doc strings for affected code have been updated.
    N/A
  • High-Level Documentation: The README is updated, as appropriate.
  • Change Log: New features or bug fixes have been added to the change log in the README.

README.md Outdated
@@ -330,7 +330,17 @@ Here are some features we're planning to add in the future:

## Changelog

### v0.6.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the version is repeated. Also, please format the bug fix in a consistent manner with previous bug fixes.

help="Level to set the root logging module to",
)
parser.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd try to pull the version number out of setup.py somehow, so we don't have to update it in two places (we'll probably forget if we have to).

n2y/database.py Outdated
if self._children is None:
self._children = self.client.get_database_pages(self.notion_id, filter, sort)
return self._children
if not filter:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the motivation for these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the database is pulled from the cache and a filter has already been run on it, but you want it to run a different filter it won't. Thus, documents/510k was just giving me the same results as documents/dhf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an intermediate variable like loading_from_cache = filter != None or something similar to capture why we're branching?

@sHermanGriffiths sHermanGriffiths requested review from johndgiese and bimbashrestha and removed request for johndgiese November 23, 2022 22:56
- add `get_children()` method to databases and pages in order to update `Database._children` and `Page._children` manually
- add `Client.copy_notion_database_children()` which allows users to copy a list of children (pages) into another database
- correct `Client.append_child_notion_blocks()` (it now copies database children the appended child_databases)
if tupled_filter not in self._filtered_children:
self._filtered_children[tupled_filter] = {}
if tupled_sort not in self._filtered_children[tupled_filter]:
self._filtered_children[tupled_filter][tupled_sort] = \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you can avoid the \ by formatting it this way

self._filtered_children[tupled_filter][tupled_sort] = self.client.get_database_pages(
    self.notion_id, filter, sort
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that for flake8, I tried your suggestion first but it would make the line 101 characters

@@ -27,6 +28,10 @@ def main(raw_args, access_token):
"--verbosity", '-v', default='INFO',
help="Level to set the root logging module to",
)
parser.add_argument(
"--version", action='version', version=pkg_resources.require("n2y")[0].version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay! thanks for adding this

n2y/notion.py Outdated
{
key: value
for (key, value) in child.items()
if key not in [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's extract out this array into a variable at the top or a constant somewhere

Copy link
Member

@bimbashrestha bimbashrestha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Minor comments but good otherwise!

@sHermanGriffiths sHermanGriffiths merged commit 1fda6c9 into main Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants