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

v.1.0 ready for your review #69

Merged
merged 13 commits into from
Sep 13, 2022
13 changes: 11 additions & 2 deletions n2y/blocks.py
Original file line number Diff line number Diff line change
Expand Up @@ -570,8 +570,17 @@ def to_pandoc(self):
return self.children_to_pandoc()


class LinkToPageBlock(WarningBlock):
pass
class LinkToPageBlock(Block):
# Invoking warning, for backward compatibility with existing
# installations, which will lack the plugin.
# TODO: right?

WarningBlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.


# It is expected that the functionality will be overwritten by an internal plugin, if
# such plugin is included in the client's specific distribution




def render_with_caption(content_ast, caption_ast):
Expand Down
9 changes: 9 additions & 0 deletions n2y/notion_mocks.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ def mock_file(url):
}


def mock_link_to_page_block(linked_page_id):
johndgiese marked this conversation as resolved.
Show resolved Hide resolved
# TODO: This function is trivial and I believe unnecessary, as
# the representation of the block itself, in this case, is trivial. QUESTION Right?
return {
'id': linked_page_id,
'type': "page_id",
}


def mock_property_value(property_value_type, content):
return {
'id': mock_id(),
Expand Down
38 changes: 38 additions & 0 deletions n2y/plugins/linktopage.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import logging

from n2y.blocks import Block

logger = logging.getLogger(__name__)

class LinkToPage(Block):
Copy link
Contributor

@johndgiese johndgiese Sep 9, 2022

Choose a reason for hiding this comment

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

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 )

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"""
Replace page link with the content of the linked-to page.

"""
def __init__(self, client, notion_data, page, get_children=False):

super().__init__(client, page, notion_data, get_children)

def to_pandoc(self):
assert self.linked_page_id is not None

try:
self.client.get_page(self.linked_page_id)
# Although out of the scope of the project at hand, get_page_or_database()
johndgiese marked this conversation as resolved.
Show resolved Hide resolved
# could be used in place of get_page, to cover the linked database case.
# Might be an unnecessary step here.
self.rich_text = self.client.wrap_notion_rich_text_array(self.notion_data["rich_text"], self)

# It's all about calling the proper method.
content = self.rich_text.to_pandoc()

except PermissionError:
johndgiese marked this conversation as resolved.
Show resolved Hide resolved
msg = 'Permission denied when attempting to access linked page having id [%s]'
logger.warning(msg, self.notion_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

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).



notion_classes = {
"blocks": {
"link_to_page": LinkToPage,
}
}
55 changes: 55 additions & 0 deletions n2y/test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import logging
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense! I think once you inherit from n2y.blocks.LinkToPageBlock that you'll be able to load your plugin.


from n2y import notion

from n2y.plugins import linktopage

"""
For testing only
Launch n2y programmatically, with specified arguments.
"""

logging.basicConfig()

global logger

logger = logging.getLogger(__name__)

logger.critical("At least I'm logging!")


# Constants to be put in .env file or the equivalent. But then again, this is just a test.
access_token = "secret_gke3p6v6mCWl7UmEjnTAoTCet3BxLQaWZeSWicWA1a8"
linked_page_id = "56850767f7b645baaffddfc1ff617db1"
plugin_id = 'n2y.plugins.linktopage'


# Just curious to have a look at these data elements.
raw_args = [linked_page_id, '--plugin', plugin_id]
#-> Nice! a tidy string of argument values, including our own linked_page_id.

# Another way to initialize arguments to our liking.
args = raw_args

# Bare bones client.
cl = notion.Client(
access_token
)

# Now we are able to instatiantate our page object.
cl.get_page(linked_page_id)


page = cl.get_page(linked_page_id)

# Here we have a notion type of child_page and the notion page id that we fed it.
cl.get_block(linked_page_id, page)

# This one gets it from Client, the top class.
## using the Client.get_block method, as above
## Now just to insert it into the page.



x = linktopage

58 changes: 57 additions & 1 deletion tests/test_blocks.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
CodeBlock, BulletList, OrderedList, Decimal, Period, Meta, Pandoc, Link,
HorizontalRule, BlockQuote, Image, MetaString, Table, TableHead, TableBody,
TableFoot, RowHeadColumns, Row, Cell, RowSpan, ColSpan, ColWidthDefault, AlignDefault,
Caption, Math, DisplayMath
Caption, Math, DisplayMath,
johndgiese marked this conversation as resolved.
Show resolved Hide resolved
)

from n2y.notion import Client
Expand Down Expand Up @@ -255,6 +255,62 @@ def test_code_block():
assert markdown == "``` javascript\nconst a = 3\n```\n"


def test_link_to_page_block():

# As I've been asked to update the end-to-end tests, but not specifically the unit tests
johndgiese marked this conversation as resolved.
Show resolved Hide resolved
# changes to this file are out of scope. Right?


mock_page_id = "23143143243241423"

# This test is not about representing the block itself, which is only a page link.
# It's about testing that a mock page, linked to by a mock block, is processed as expected.

# Elaboration: As the link_to_page block is trivial, containing only the value of the link itself.
# And since representation of the page link in the pandoc output is not necessary --
# It is the contents of the linked page that is potentially
# represented, not the link to the page -- testing here would logically be limited to checking
# for proper conversion of the linked page. However, such a test would be redundant,
# as already handled in test-end-to-end.py

# Create the mock block of the linking page.
notion_block = mock_block("link_to_page", {
Copy link
Contributor

Choose a reason for hiding this comment

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

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}"})

"type": "link_to_page",
"link_to_page": { "type": "page_id", "page_id": "{mock_page_id}" }
})


# TODO: QUESTION What type of Notion block is a page? Below, I use a
# paragraph block. Is that correct?

# Create a mock of the page to which the link points.
notion_page = mock_block("paragraph", {
"rich_text": [{
"type": "text",
"text": {
"content": "Test content",
"link": null
}
}],
})

# Test the block itself.
pandoc_ast, markdown = process_block(notion_block)
# TODO: fill in for None, if I am wrong and unit tests are required.
assert pandoc_ast == None
assert markdown == None

# Test the mock page to which the mock link_to_page links.
# We will be testing
pandoc_ast, markdown = process_block(notion_page)
# TODO: fill in for None, if I am wrong and unit tests are required.
assert pandoc_ast == None
assert markdown == None





def test_table_block():
parent = mock_block("table", {
"table_width": 2,
Expand Down
19 changes: 19 additions & 0 deletions tests/test_end_to_end.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ def test_all_properties_database():


def test_all_blocks_page_to_markdown(tmp_path):

'''
The page can be seen here:
https://fresh-pencil-9f3.notion.site/Test-Page-5f18c7d7eda44986ae7d938a12817cc0
Expand Down Expand Up @@ -212,8 +213,12 @@ def test_all_blocks_page_to_markdown(tmp_path):
assert "``` javascript\nCode Block\n```" in document_as_markdown
assert lines.count("This is a synced block.") == 2
assert "This is a synced block from another page." in lines
assert "This page is not shared with the integration." in lines

assert all(column_strings_in_lines) or (column_string in lines)


johndgiese marked this conversation as resolved.
Show resolved Hide resolved

# a bookmark with a caption and without
assert "<https://innolitics.com>" in lines
assert "[Bookmark caption](https://innolitics.com)" in lines
Expand Down Expand Up @@ -256,6 +261,8 @@ def test_simple_page_to_markdown():
assert "Page content" in document_as_markdown




def test_builtin_plugins(tmp_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@johndgiese johndgiese Sep 9, 2022

Choose a reason for hiding this comment

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

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.

'''
The page can be seen here:
Expand Down Expand Up @@ -296,3 +303,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():
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


# This is exctly what test_all_blocks_page_to_markdown() does, so it
# doesn't make sense to do it again, but what else can we do to test
# this capability?
# Unless we come up with a real purpose for this test, it probably doesn't make sense.
object_id = '6670dc17a7bc4426b91bca4cf3ac5623'
status, document_as_markdown, _ = run_n2y([object_id])
assert status == 0
assert "Page content" in document_as_markdown