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

New feature: support for {include} syntax. Fixes #1902. #1909

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@atodorov
Contributor

atodorov commented Feb 11, 2016

The new {include} syntax makes it possible to include
frequently used text snippets into your content.

@avaris

This comment has been minimized.

Show comment
Hide comment
@avaris

avaris Feb 18, 2016

Member

Nice idea, but it won't work as expected for non-html content. You're dumping the content of the file in place of the include. For .rst or .md (or any other parsed content), this means that it'll put raw syntax instead.

e.g. for a markdown file to be included, you'd get:

[link](http://www.example.com)

rather than

<a href="http://www.example.com">link</a>

Plus, you have no error checking at all. No graceful fallbacks. Give a wrong path by mistake and things will break down.

Member

avaris commented Feb 18, 2016

Nice idea, but it won't work as expected for non-html content. You're dumping the content of the file in place of the include. For .rst or .md (or any other parsed content), this means that it'll put raw syntax instead.

e.g. for a markdown file to be included, you'd get:

[link](http://www.example.com)

rather than

<a href="http://www.example.com">link</a>

Plus, you have no error checking at all. No graceful fallbacks. Give a wrong path by mistake and things will break down.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Feb 18, 2016

Contributor

@avaris - Good points, thanks for the feedback.

Yes I dump raw content in place of the include, this will be a problem for inclusions between files of different types. I'll try to render the new content to HTML before including it. How does it sounds ?

On the second point - error checking - will add it. Is there any particular exceptions or methods to handle this. If possible point me to some place in the code which has good error checking and fallbacks.

Contributor

atodorov commented Feb 18, 2016

@avaris - Good points, thanks for the feedback.

Yes I dump raw content in place of the include, this will be a problem for inclusions between files of different types. I'll try to render the new content to HTML before including it. How does it sounds ?

On the second point - error checking - will add it. Is there any particular exceptions or methods to handle this. If possible point me to some place in the code which has good error checking and fallbacks.

Show outdated Hide outdated pelican/contents.py
)
)
with pelican_open(path) as text:

This comment has been minimized.

@avaris

avaris Feb 19, 2016

Member

This will throw an exception if path doesn't exist or isn't a file. You could use the same method done in _update_content. Show a warning and ignore the replace.

@avaris

avaris Feb 19, 2016

Member

This will throw an exception if path doesn't exist or isn't a file. You could use the same method done in _update_content. Show a warning and ignore the replace.

@avaris

This comment has been minimized.

Show comment
Hide comment
@avaris

avaris Feb 19, 2016

Member

Also, if I'm not mistaken, pelican will try to read your includes as regular content files. Either it'll will succeed and they will be considered for Page/Article or it'll fail and spit a bunch of warnings. I don't think any of these outcomes is what you want. They should be ignored, IMO.

Member

avaris commented Feb 19, 2016

Also, if I'm not mistaken, pelican will try to read your includes as regular content files. Either it'll will succeed and they will be considered for Page/Article or it'll fail and spit a bunch of warnings. I don't think any of these outcomes is what you want. They should be ignored, IMO.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 11, 2016

Contributor

@avaris - it took me a while to get back to this feature but I'm back on it. Regarding your last comment:

Yes, Pelican tries to read the included files as regular content. In most of the cases it fails b/c I don't provide a title for example. I'll be happy to ignore them if there is an easy way to do so. Is there such a way?

If not then I propose to have an additional setting, something like IGNORE_DIRS which are directories to ignore reading content from and instruct the user to place their include files there. I'm not sure if this won't conflict with pelican_openthough.

I'll look at your previous comment as well and update this PR when ready.

Contributor

atodorov commented Mar 11, 2016

@avaris - it took me a while to get back to this feature but I'm back on it. Regarding your last comment:

Yes, Pelican tries to read the included files as regular content. In most of the cases it fails b/c I don't provide a title for example. I'll be happy to ignore them if there is an easy way to do so. Is there such a way?

If not then I propose to have an additional setting, something like IGNORE_DIRS which are directories to ignore reading content from and instruct the user to place their include files there. I'm not sure if this won't conflict with pelican_openthough.

I'll look at your previous comment as well and update this PR when ready.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 11, 2016

Contributor

IGNORE_FILES is the setting I needed earlier. Just listing my included content there stops Pelican from trying to render it and produce an error. I've added a section about this in the docs.

Now looking into the final issue from 2nd comment - how to properly include content between files of various formats.

UPDATE: I'm trying to use Readers.read_file instead of the pelican_open statement both to handle recursive includes and to actually render the content to HTML from whatever the input format is. I'm trying to pass it a custom content_class but so far this produces objects with empty content. I suspect this has something to do with the fact that I don't have a template for this new class but I don't know how to proceed at this point. Please advise.

Contributor

atodorov commented Mar 11, 2016

IGNORE_FILES is the setting I needed earlier. Just listing my included content there stops Pelican from trying to render it and produce an error. I've added a section about this in the docs.

Now looking into the final issue from 2nd comment - how to properly include content between files of various formats.

UPDATE: I'm trying to use Readers.read_file instead of the pelican_open statement both to handle recursive includes and to actually render the content to HTML from whatever the input format is. I'm trying to pass it a custom content_class but so far this produces objects with empty content. I suspect this has something to do with the fact that I don't have a template for this new class but I don't know how to proceed at this point. Please advise.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov May 27, 2016

Contributor

ping, I've rebased to master. This is ready for code review.

Contributor

atodorov commented May 27, 2016

ping, I've rebased to master. This is ready for code review.

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Sep 30, 2016

Member

Fellow @getpelican/reviewers: Would you please take a look a this PR and provide feedback?

Member

justinmayer commented Sep 30, 2016

Fellow @getpelican/reviewers: Would you please take a look a this PR and provide feedback?

@leotrs

leotrs approved these changes Dec 13, 2016

Show outdated Hide outdated pelican/contents.py
)
if path not in self._context['filenames']:
unquoted_path = path.replace('%20', ' ')

This comment has been minimized.

@leotrs

leotrs Dec 13, 2016

What about using unquote?

@leotrs

leotrs Dec 13, 2016

What about using unquote?

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Jun 1, 2017

Contributor

@getpelican/reviewers can you review?

Contributor

atodorov commented Jun 1, 2017

@getpelican/reviewers can you review?

@ingwinlu

This comment has been minimized.

Show comment
Hide comment
@ingwinlu

ingwinlu Jun 2, 2017

Contributor
Contributor

ingwinlu commented Jun 2, 2017

@avaris

This comment has been minimized.

Show comment
Hide comment
@avaris

avaris Jun 4, 2017

Member

I agree with @ingwinlu about circular imports. Instead you can pass in the reader to the __init__ of Content from Readers.read_file (here).

A check for infinite recursion might be needed as well (if an import imports itself).

Also, no ads please :).

Member

avaris commented Jun 4, 2017

I agree with @ingwinlu about circular imports. Instead you can pass in the reader to the __init__ of Content from Readers.read_file (here).

A check for infinite recursion might be needed as well (if an import imports itself).

Also, no ads please :).

@justinmayer

Many thanks for following up, Alexander. Would you please make the changes suggested by @ingwinlu and @avaris? Also, it seems that the pelican/tests/content/include/include{1,2,3,4}.html files don't contain a newline at the end of the file.

atodorov added some commits May 26, 2016

New feature: support for {include} syntax. Fixes #1902.
The new {include} syntax makes it possible to include
frequently used text snippets into your content.
@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Jun 10, 2017

Contributor

@avaris I've implemented your proposal to pass Readers() to Content.__init__() and I broke test_cache.py. I think we need to create the readers like I do in test_content.py and that probably needs to go to generators.py or somewhere in test_cache.py but ATM I can't figure out where exactly.

Contributor

atodorov commented Jun 10, 2017

@avaris I've implemented your proposal to pass Readers() to Content.__init__() and I broke test_cache.py. I think we need to create the readers like I do in test_content.py and that probably needs to go to generators.py or somewhere in test_cache.py but ATM I can't figure out where exactly.

@avaris

This comment has been minimized.

Show comment
Hide comment
@avaris

avaris Jun 11, 2017

Member

@atodorov, the more I look at it, the more I think that it's better to resolve the {include} at the reader level, before it reaches to the Content. I know, this will be a major overhaul, but I think it will be cleaner that way.

Member

avaris commented Jun 11, 2017

@atodorov, the more I look at it, the more I think that it's better to resolve the {include} at the reader level, before it reaches to the Content. I know, this will be a major overhaul, but I think it will be cleaner that way.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Jun 12, 2017

Contributor

@atodorov, the more I look at it, the more I think that it's better to resolve the {include} at the reader level, before it reaches to the Content. I know, this will be a major overhaul, but I think it will be cleaner that way.

I can give it a try but honestly my Pelican knowledge is a bit rusty ATM so you will have to tell me where to look at or give me some hints as to how you see things being implemented.

Contributor

atodorov commented Jun 12, 2017

@atodorov, the more I look at it, the more I think that it's better to resolve the {include} at the reader level, before it reaches to the Content. I know, this will be a major overhaul, but I think it will be cleaner that way.

I can give it a try but honestly my Pelican knowledge is a bit rusty ATM so you will have to tell me where to look at or give me some hints as to how you see things being implemented.

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Mar 23, 2018

Member

@avaris: Any chance you could point Alexander in the direction you're thinking about this PR?

Member

justinmayer commented Mar 23, 2018

@avaris: Any chance you could point Alexander in the direction you're thinking about this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment