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

Fix docs for abstract_delimiter #202

Closed
wants to merge 1 commit into from
Closed

Conversation

craigbox
Copy link

The copy and paste from abstract_chars_count didn't match abstract_delimiter.

If you tried -1:

ERROR   -  Config value 'plugins': Plugin 'rss' option 'abstract_delimiter': Expected type: <class 'str'> but received: <class 'int'>

"" works well.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 11, 2023
@Guts Guts self-assigned this Sep 12, 2023
@YDX-2147483647
Copy link
Contributor

Here're the corresponding implementation (to help the maintainer review).

class RssPluginConfig(Config):
"""Configuration for RSS plugin for Mkdocs."""
abstract_chars_count = config_options.Type(int, default=160)
abstract_delimiter = config_options.Type(str, default="<!-- more -->")

def get_description_or_abstract(
self, in_page: Page, chars_count: int = 160, abstract_delimiter: str = None
) -> str:
"""Returns description from page meta. If it doesn't exist, use the \
{chars_count} first characters from page content (in markdown).
:param Page in_page: page to look at
:param int chars_count: if page.meta.description is not set, number of chars \
of the content to use. Defaults to: 160 - optional
:param str abstract_delimiter: description delimeter, defaults to None
:return: page description to use
:rtype: str
"""

(↑delimeter is misspelled)

The following if will fail if abstract_delimiter is "", therefore it will behave as if the delimiter does not exist.

elif (
abstract_delimiter
and (
excerpt_separator_position := in_page.markdown.find(abstract_delimiter)
)
> -1
):
return markdown.markdown(
in_page.markdown[:excerpt_separator_position],
output_format="html5",
)

By the way, abstract_delimiter: "" is not tested yet.

@Guts
Copy link
Owner

Guts commented Jan 18, 2024

Hi @craigbox ,

Thanks for reporting.
Your PR is definitely at the top of my list for my next dev time on this project.

I would prefer that the code works as expected and documented, instead of changing the documentation.
I'll look into what you already identified @YDX-2147483647. Thanks you a lot for making my life easier 🤗

@craigbox in the meantime can you rebase on origin/main and try again please?

@craigbox
Copy link
Author

Rebased and pushed. (The spelling of "delimiter" has already been fixed.)

@Guts Guts mentioned this pull request Apr 22, 2024
Guts added a commit that referenced this pull request Apr 23, 2024
Patches sent by mail by @tiosgz. Applied with `git am`. It's my very
first time in this kind of [git email
workflow](https://git-send-email.io/). Pfiu !.

Command used:

```sh
git am --3way --ignore-space-change v2-0001-fix-retrieving-article-description.patch
git am --3way --ignore-space-change v2-0002-docs-configuration-fix-update-abstract_-chars_cou.patch
git am --3way --ignore-space-change v2-0003-tests-add-test-cases-for-abstract_delimiter.patch
```

Supersedes #202

cc @craigbox  @YDX-2147483647
@Guts
Copy link
Owner

Guts commented Apr 24, 2024

I close here since #268 fixed the logic behind.

@Guts Guts closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants