8000 Provide unlimited abstract length option by liang2kl · Pull Request #88 · Guts/mkdocs-rss-plugin · GitHub
[go: up one dir, main page]
Skip to content

Provide unlimited abstract length option #88

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.

Sign up for GitHub

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

Jump to bottom
Merged
Guts merged 13 commits into from
Sep 14, 2021
Merged

Provide unlimited abstract length option #88

Guts merged 13 commits into from
Sep 14, 2021

Conversation

@liang2kl
Copy link
Contributor
@liang2kl liang2kl commented Sep 9, 2021 •
edited
Loading

It is useful to provide an option for users who want to publish the whole article, to be read directly on rss readers.

  • Set default option for abstract_chars_count to None
  • Returns full text when abstract_chars_count is None
  • Change on_page_markdown to on_page_content to retrieve final html content for rss description

@Guts Guts self-assigned this Sep 9, 2021
@Guts Guts added the enhancement New feature or request label Sep 9, 2021
@Guts
Copy link
Owner
Guts commented Sep 9, 2021

Thanks for your proposal.

It is useful to provide an option for users who want to publish the whole article, to be read directly on rss readers.

Agreed!

Set default option for abstract_chars_count to None

I prefer to keep it as an option to avoid a breaking change for existing installations.

Returns full text when abstract_chars_count is None

Good move. Maybe when abstract_chars_count==-1, also to avoid a breaking change for existing installations.

Change on_page_markdown to on_page_content to retrieve final html content for rss description

Are you totally sure thath it's not breaking something? If you take the full content, it's ok and easy-to-go. But if you specify a length, you'll need to manage a valid truncated HTML and it's not so simple.

Regarding failing tests, it's on mkdocs-minify-plugin --> jsmin side. I'm monitoring the upstream PR.

@liang2kl
Copy link
Contributor Author
liang2kl commented Sep 10, 2021

Maybe when abstract_chars_count==-1, also to avoid a breaking change for existing installations.

Agreed. I will update shortly.

But if you specify a length, you'll need to manage a valid truncated HTML and it's not so simple.

In my pr, if the length is specified, the Markdown string, instead of HTML, will be used (which is the same as before, as on_page_markdown will not provide html string at all), so it should be fine.

@liang2kl liang2kl changed the title Set unlimited default abstract length Provide unlimited default abstract length Sep 10, 2021
@liang2kl liang2kl changed the title Provide unlimited default abstract length Provide unlimited abstract length option Sep 10, 2021
@Guts
Copy link
Owner
Guts commented Sep 10, 2021

In my pr, if the length is specified, the Markdown string, instead of HTML, will be used (which is the same as before, as on_page_markdown will not provide html string at all), so it should be fine.

Great. I'll give it a closer look in the next days.

In the meanwhile, could you include the documentation update please?

And check if you follow the contributing guidelines. Thanks!

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 10, 2021
@liang2kl
Copy link
Contributor Author
liang2kl commented Sep 10, 2021

The docs have been updated.

@codecov
Copy link
codecov bot commented Sep 10, 2021 •
edited
Loading

Codecov Report

Merging #88 (2dc2364) into main (65db2c2) will decrease coverage by 0.13%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
- Coverage   70.35%   70.22%   -0.14%     
==========================================
  Files           5        5              
  Lines         307      309       +2     
  Branches       59       60       +1     
==========================================
+ Hits          216      217       +1     
  Misses         63       63              
- Partials       28       29       +1
Impacted Files Coverage Δ
mkdocs_rss_plugin/util.py 60.86% <27.27%> (-0.14%) ⬇️
mkdocs_rss_plugin/plugin.py 78.12% <100.00%> (ø)

Guts
Guts requested changes Sep 10, 2021
View reviewed changes
@liang2kl liang2kl requested a review from Guts September 11, 2021 02:29
@Guts
Copy link
Owner
Guts commented Sep 12, 2021

Hi @liang2kl,

I've just reviewed it deeper and I've got two concerns:

  • the output HTML should be included into a CDATA section since HTML into XML is not a straight forward thing (see: https://www.w3schools.com/XML/dom_cdatasection.asp), no?
  • the embedded HTML is not valid because main tags are missing: <html><body></body></html>. Is that the behavior you want?

@liang2kl
Copy link
Contributor Author
liang2kl commented Sep 13, 2021 •
edited
Loading

I don't have much understanding about the standards, but it seems that in your previous implementation, you deal it the same way, generating html content without the main tags, and just embedding the html into xml?

@Guts Guts merged commit f396f10 into Guts:main Sep 14, 2021
@Guts
Copy link
Owner
Guts commented Sep 20, 2021

Now deployed as part of the version 0.18.0:

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

Reviewers

@Guts Guts Guts approved these changes

Assignees

@Guts Guts

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Guts
0