Rewrite and Refractor #20
Conversation
| self.title_count -= 1 | ||
| if self.title_count < 0: |
There was a problem hiding this comment.
counts should count upwards.
Also, what if the first section does not have text?
Example: https://docs.wpilib.org/en/latest/docs/software/driverstation/manually-setting-the-driver-station-to-start-custom-dashboard.html
If we start counting sections, we should either:
- Start counting at the first section containing text
- Only count sections containing text.
There was a problem hiding this comment.
I forgot to ask about this part in the discord, but this is another part I wasn't really sure about.
Right now the idea is the user can enter how many sections to search, but another possibility would be to only grab the first section that isn't empty.
I agree that regardless empty sections should be ignored.
Not sure if we should put in the titles of empty sections or ignore all titles as it is now.
Edit: downwards counting was to reduce variable count if it is something the user inputs, probably will have to change to make the changes above.
There was a problem hiding this comment.
Regarding titles, the options I see are either:
- Only include titles of sections with text
- Ignore all titles.
Either is fine really.
Regarding counting sections:
Counting sections falls counter to description length. If the user specifies 300 characters, then it may not be intuitive as to why a lot less characters were included.
I'm fine with providing options for both description_length and num_sections but it would need to be clear in the readme that the amount of text grabbed is pseudo_min(description_length, num_sections).
There was a problem hiding this comment.
Regarding the counting sections, my train of thought is that ogp:description is meant to be a small short description of the page, just grabbing the first X chars means it will always just be everything that can fit, while grabbing the first paragraph will most likely be some kind of either description of the page, or snippet of the contents of the page.
I wouldn't be opposed to having it only grab the first section with text and renaming the desc_len to max_desc_len.
The other option would be to have that configurable, where it can grab the first 300 chars and ignore all titles, or do as mentioned above.
We could get into more complex title impls, but I feel like the thing I said above should be the default and be the best in most use cases.
There was a problem hiding this comment.
Grabbing just one section isn't enough. The previous impl by a Sphinx Maintainer was along the lines of pseudomin(3 sections, 200 char). I agree with your argument for counting sections. I just think that both should be options.
| from sphinx.application import Sphinx | ||
| from .event_handler import html_page_context | ||
|
|
||
| # todo: rtd option to grab READTHEDOCS_LANGUAGE and READTHEDOCS_VERSION for link |
There was a problem hiding this comment.
We can put these TODOs in a separate PR after this is merged.
There was a problem hiding this comment.
Yea, those are going to be gone before I remove it from being a draft, just waiting for an opinion on the lists and sections to close that part off
|
Regarding list handling: The one edge case I can think of that needs to be clarified: It's a list in the first section of text. |
|
My thought is to go back to a more similar impl to what was before, just actually format lists in. This is, numbered lists are treated like a normal list, sublists have a bit of special formatting, the rest is just comma-separated elements. |
|
I don't really have a strong opinion on lists other than the point about index pages I made above. As long as that works, do what you think is best. |
| add_tag_from_config(context, app.config, "site_name") | ||
| context["metatags"] += make_tag("description", visitor.description) | ||
| add_tag_from_config(context, app.config, "image") | ||
| add_tag_from_config(context, app.config, "image:alt") |
There was a problem hiding this comment.
I don't like the mixed usage of context["metatags"] += and add_tag_from_config. I think add_tag_from_config reduces readability and should be removed.
| try: | ||
| desc_len = int(app.config["ogp_description_length"]) | ||
| except ValueError: | ||
| desc_len = 200 |
There was a problem hiding this comment.
Constants shouldn't be defined twice.
Since app.add_config_value("ogp_description_length", 200, "html") is already in __init__.py, this isn't necessary.
Part of #11.
TODOs can be ignored (will be removed).