v.1.0 ready for your review #69
Conversation
This commit is a draft solution to the assignment. Comments are intended for author and reviewer. There are several loose ends, or undeveloped blocks.
n2y/plugins/linktopage.py
Outdated
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| class LinkToPage(Block): |
There was a problem hiding this comment.
The plugin class needs to inherit from the existing n2y.blocks.LinkToPageBlock class in order for this to work. (This could be made more explicit, but this is what's discussed in bullet point 2 in the README here https://github.com/innolitics/n2y#plugins )
There was a problem hiding this comment.
I would have thought you'd get an exception raised from here because this is not subclassing n2y.blocks.LinkToPageBlock:
https://github.com/innolitics/n2y/blob/main/n2y/notion.py#L149-L155
Were you seeing this exception? If not, this is a bug in itself.
There was a problem hiding this comment.
Yes, I do understand that the instructions were that the plugin should inherit from the LinkToPageBlock.
I brought up some issues to Bimba concerning this inheritance. I can elaborate. Anyway, he suggested that I could just inherit from Block. Sounded good to me, so as not to disturb the original functioning of the class for existing customers. I can explain to you. The issue is described in my comments on files of previous commits.
The only exception that I saw when trying to subclass n2y.blocks.LinkToPageBlock was that if the parent of that class were WarningBlock, then the interpreter complained that I wasn't overriding WarningBlock, as I recall. I would need to recreate the error in order to tell you more.
There was a problem hiding this comment.
I see, yeah @bimbashrestha @idolcemia as written, I believe you must inherit from LinkToPageBlock. I don't think you should need to modify the n2y.blocks.LinkToPageClass at all. Your class will presumably override both the __init__ and to_pandoc methods, thus removing any warning functionality that you don't want in your class. Since you won't be touching n2y.blocks.LinkToPageClass at all, you can be sure it won't affect other people using it.
I'm surprised that it would complain about you not overriding WarningBlock. That sounds like a bug in the error handling code. If you happen to reproduce it, please copy the stack trace and let me know.
n2y/plugins/linktopage.py
Outdated
|
|
||
| except PermissionError: | ||
| msg = 'Permission denied when attempting to access linked page having id [%s]' | ||
| logger.warning(msg, self.notion_data) |
There was a problem hiding this comment.
I don't think self.notion_data is what you'd want to use here, since it will contain a lot more than the linked page id. Also, you'll want to be sure to include the URL to the LinkToPage block itself so that people who run into this error can quickly track down it's cause and fix it (by clicking on the URL and being taken to where the block is).
n2y/blocks.py
Outdated
| # installations, which will lack the plugin. | ||
| # TODO: right? | ||
|
|
||
| WarningBlock() |
There was a problem hiding this comment.
As best I can tell, you're instantiating a new instance of the WarningBlock class within the class definition block. The resulting instance is immediately discarded since it doesn't do anything, but even if it were assigned to something, I'm not sure what the intent would be.
@idolcemia could you expand a bit on what you're expecting this to do and why you'd think it would work?
There was a problem hiding this comment.
@idolcemia I saw you modified the code, but I would still like to know your answer to my question. I think it's important because, based on the code, I suspect you have some sort of misunderstanding regarding how python classes work. That said, it may also be that I have a misunderstanding of some sort.
n2y/test.py
Outdated
| @@ -0,0 +1,55 @@ | |||
| import logging | |||
There was a problem hiding this comment.
It looks like it's a test script you're using locally. I'd suggest not committing these sorts of files to the repo, as this sort of testing should really be handled in the automated tests.
There was a problem hiding this comment.
Absolutely. Next time I will upload this as a separate file in another fashion if I want you to see it. I was unable to get the plugin loaded in the proper course of n2y, so I had to write this module to test it.
test.py is not a test module, never mind the name. It is a work around for my being unable to load as a plugin my plugin.
There was a problem hiding this comment.
That makes sense! I think once you inherit from n2y.blocks.LinkToPageBlock that you'll be able to load your plugin.
tests/test_blocks.py
Outdated
| # as already handled in test-end-to-end.py | ||
|
|
||
| # Create the mock block of the linking page. | ||
| notion_block = mock_block("link_to_page", { |
There was a problem hiding this comment.
Nice job identifying the need to use mock_block here, but you're not calling it correctly. See the code for how mock_block is implemented to see why:
https://github.com/innolitics/n2y/blob/main/n2y/notion_mocks.py#L94
In particular, you can just use mock_block("link_to_page", {"type": "page_id", "page_id": "{mock_page_id}"})
| @@ -256,6 +261,8 @@ def test_simple_page_to_markdown(): | |||
| assert "Page content" in document_as_markdown | |||
|
|
|||
|
|
|||
|
|
|||
|
|
|||
| def test_builtin_plugins(tmp_path): | |||
There was a problem hiding this comment.
I was expecting that you would update the plugin end to end tests (i.e., this one where the comment is). See the issue for more details and let me know if it's not clear.
There was a problem hiding this comment.
My update is the method def test_links_to_pages() inside /tests/test_end_to_end.py What did I miss? My comments are about how best, if at all, to test this function, whose operation is trivial.
There was a problem hiding this comment.
Per the instructions in the issue, I was hoping you'd "Update the existing plugin end-to-end test". I.e., you would update the test_builtin_plugins. I should have pointed to this specific test name to avoid confusion. This end to end test already loads all of the other builtin plugins and tests that they work (more or less). You can just add your new plugin and update the associated notion page to test it.
I don't think you should need to add any new end to end tests (i.e., you can probably remove test_links_to_pages) or modify the test_all_blocks_page_to_markdown test (that test is meant to test how things work without any plugins.
There was a problem hiding this comment.
@idolcemia as you stated, I don't believe this is working. I left a number of comments which should help you identify why.
I realize this is a work in progress PR, but please note that most of the comments you've added should not be included in the final PR. Also, note that there are a lot of formatting issues that you'll need to fix in order to pass the flake8 check.
Let me know if you have any questions about my comments.
End of line characters removed per flake8
|
I've done everything, I think. But I am still unable to test., for this reason: When I look at page.blocks.children, the linked page block is represented as an unsupported block. Since the blocks are not retrieved, they cannot be handled. This condition predates my work. I may be missing something of course. |
n2y/blocks.py
Outdated
| try: | ||
| page = self.client.get_page_or_database(self.linked_page_id) | ||
| title = page.title.to_pandoc() | ||
| except PermissionError: |
There was a problem hiding this comment.
Does client.get_page_or_database throw a PermissionError when the page is not accessible?
n2y/blocks.py
Outdated
| msg = ( | ||
| "Permission denied when attempting to access linked page having id [%s]" | ||
| ) | ||
| logger.warning(msg, self.linked_page_id) |
There was a problem hiding this comment.
Please see the comment on my previous review here: #69 (comment)
I don't think you fully addressed the comment.
n2y/blocks.py
Outdated
|
|
||
| # The linked page may be outside the scope of what is accessible to n2y. | ||
| try: | ||
| page = self.client.get_page_or_database(self.linked_page_id) |
There was a problem hiding this comment.
Nice idea combining the if/else branches in my previous version of LinkToPageBlock. I think, given that the return value can be either a page or a database, the variable is misleading. I'd rename it to node as that is what we use in other places for nodes that can be either a database or a page (e.g., in main.py).
n2y/plugins/linktopage.py
Outdated
| rich_text = self.client.wrap_notion_rich_text_array( | ||
| page.notion_data["rich_text"], self) | ||
|
|
||
| except PermissionError: |
n2y/plugins/linktopage.py
Outdated
| try: | ||
| page = self.client.get_page_or_database(self.linked_page_id) | ||
|
|
||
| rich_text = self.client.wrap_notion_rich_text_array( |
There was a problem hiding this comment.
@idolcemia can you explain the reasoning behind this line of code? I don't think it really makes sense and I'm surprised that the tests are passing.
n2y/plugins/linktopage.py
Outdated
|
|
||
| else: | ||
|
|
||
| logger.warning( |
There was a problem hiding this comment.
In general, all log messages should include URLs to the block in Notion where the error can be fixed. Thus, we should include a URL to the LinkToPageBlock where the link originates. The ids of the pages are less useful to a user. Thus, please update the error message to only include a link to the original block like most of the other logger.warning calls in the code repo. You can get a sense for what I mean by prepping for logger.warning.
@idolcemia I thought I'd spelled this out pretty clearly in the original Issue:
Handle the case where the plugin doesn't have permission to access the linked page; in this case, log a warning including a link to the notion block where it occurred (grep for logger.warning for examples).
I also brought it up in my first review. Please pay attention to the details on this stuff.
tests/test_end_to_end.py
Outdated
| @@ -296,3 +338,15 @@ def test_builtin_plugins(tmp_path): | |||
| def test_missing_object_exception(): | |||
| invalid_page_id = "11111111111111111111111111111111" | |||
| assert run_n2y([invalid_page_id]) != 0 | |||
|
|
|||
|
|
|||
| def test_links_to_pages(): | |||
There was a problem hiding this comment.
I thought, per the comments in the previous review, that we were going to remove test_links_to_page, especially given the comment you have here.
Here's my previous comment about this: #69 (comment)
Please delete this test.
| ]) | ||
| status, document_as_markdown, _ = run_n2y( | ||
| [ | ||
| object_id, |
There was a problem hiding this comment.
@idolcemia in your PR you said the following:
I've done everything, I think. But I am still unable to test., for this reason:
When I look at page.blocks.children, the linked page block is represented as an unsupported block. Since the blocks are not retrieved, they cannot be handled. This condition predates my work.
Does the code in this PR produces the situation where page.blocks.children is represented by an unsupported block? If not, please push up the code that produces this condition so that I can read through it. Please include specifics so that I can reproduce the issue myself. I am a little skeptical that there is an existing bug that's causing this, so I'd like to be able to reproduce it.
There was a problem hiding this comment.
@idolcemia see my Slack comment for more details about the overall review. I don't think you've completed the last four items in the issue, so I unchecked them there. Once you push up the code that reproduces the error with the end-to-end test, then I can help debug the issue and then you'll be able to finish out the last four items.
- The two unit tests will be fixed later - Remove a spurious assertion that was added to the all blocks test - Remove a dead end to end test
Everything is written, but I'm not sure it's working.
Actually, I am pretty sure....sure there'll be many changes.
I wrote a test suite (such as it is) which I included it in this commit, as an FYI.
I wrote the test suite because I was unable to test the plugin as an actual plugin. I spent some time trying to solve what I expected to be a simple problem, but gave up to focus on the core of the assignment.
I discovered an upside to not being able to test the plugin in situ. The process of creating the test suite gave me a much deeper understanding of n2y than I would have otherwise had.
Thanks you for the opportunity to work on this project. It sent me in a lot of directions, or so it felt. And I enjoyed getting into the code base very much. I was mightily impressed by the level of work.