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

Some docs not rendering properly #10427

Closed
augustelalande opened this issue Mar 15, 2024 · 23 comments · Fixed by #10499
Closed

Some docs not rendering properly #10427

augustelalande opened this issue Mar 15, 2024 · 23 comments · Fixed by #10499
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@augustelalande
Copy link
Contributor

augustelalande commented Mar 15, 2024

https://docs.astral.sh/ruff/rules/too-many-blank-lines/ (among others) is not rendering properly, even though the markdown seems fine.

The list is not rendered as a bullet list.
image

The links to settings do not link.
image

@augustelalande augustelalande changed the title Some docs not rendering properlu Some docs not rendering properly Mar 15, 2024
@zanieb zanieb added bug Something isn't working documentation Improvements or additions to documentation labels Mar 15, 2024
@zanieb
Copy link
Member

zanieb commented Mar 15, 2024

Thanks for flagging this!

@augustelalande
Copy link
Contributor Author

augustelalande commented Mar 17, 2024

Ok so there are two issues at play here.

  1. Sentences followed by lists
list:
- item1
- item2

will not be rendered as lists by python-mardown (Python-Markdown/markdown#1442), which is what mkdocs uses to transcompile the mardown to html. Instead a newline needs to be added between the sentence and items.

list:

- item1
- item2

Annoyingly, this is different from the behavior of many other markdown processors (including the one used on github) so most people will not be expecting this.

  1. The current documentation preprocessor is very strict about linking settings, only lines under the options header, using a - list indicator will be processed and linked correctly, so what was attempted in the misbehaving doc will not work.
    for line in documentation.split_inclusive('\n') {
    if line.starts_with("## ") {
    in_options = line == "## Options\n";
    } else if in_options {
    if let Some(rest) = line.strip_prefix("- `") {
    let option = rest.trim_end().trim_end_matches('`');
    match Options::metadata().find(option) {
    Some(OptionEntry::Field(field)) => {
    if field.deprecated.is_some() {
    eprintln!("Rule {rule_name} references deprecated option {option}.");
    }
    }
    Some(_) => {}
    None => {
    panic!("Unknown option {option} referenced by rule {rule_name}");
    }
    }

@augustelalande
Copy link
Contributor Author

Regarding issue 1, it is a fairly easy fix to change the comments in the code docs, but might be difficult to ensure all of them are fixed.

Secondly, because the behavior is so different to what people are used to I feel something should be done to prevent this in the future.

@augustelalande
Copy link
Contributor Author

Regarding issue 2, the doc prepossor could be expanded to accept more patterns, but it depends how frequent this type of links will appear outside the options headings.

@zanieb
Copy link
Member

zanieb commented Mar 17, 2024

Yeah this is a little annoying, maybe we can run our markdown formatter on the generated documentation? I wonder if it adds spacing.

In the meantime, it seems like adding the missing spaces makes sense.

I'm not sure about the automatic linking, @charliermarsh do you have context on the original intent of limiting the scope?

@charliermarsh
Copy link
Member

I think it’s just because the original intent was for that to support the Options section which always has that format. We can expand the scope a bit though.

@augustelalande
Copy link
Contributor Author

Yeah this is a little annoying, maybe we can run our markdown formatter on the generated documentation? I wonder if it adds spacing.

In the meantime, it seems like adding the missing spaces makes sense.

I'm not sure about the automatic linking, @charliermarsh do you have context on the original intent of limiting the scope?

Are you already running a markdown formatter in the project?

@augustelalande
Copy link
Contributor Author

If not we could try running https://github.com/executablebooks/mdformat which seems to do the trick.

@augustelalande
Copy link
Contributor Author

For the option processing we could match this regex \[`(.*?)\`\]\[\1\] to generate the option reference?

@zanieb
Copy link
Member

zanieb commented Mar 17, 2024

We use a couple

https://github.com/astral-sh/ruff/blob/main/.pre-commit-config.yaml#L20-L41

It might require a tweak to run them on the generated documentation

@augustelalande
Copy link
Contributor Author

augustelalande commented Mar 18, 2024

@zanieb I started implementing a check using mdformat, but as it is right now pretty much every doc has multiple violations (mostly missing or extra newlines). For example:

Rule `zip-without-explicit-strict` docs are not formatted. Either format the rule or add to `KNOWN_FORMATTING_VIOLATIONS`. The section should be rewritten to:
---

+++

@@ -1,12 +1,15 @@

 # zip-without-explicit-strict (B905)
+
 Derived from the **flake8-bugbear** linter.

 Fix is always available.

 ## What it does
+
 Checks for `zip` calls without an explicit `strict` parameter.

 ## Why is this bad?
+
 By default, if the iterables passed to `zip` are of different lengths, the
 resulting iterator will be silently truncated to the length of the shortest
 iterable. This can lead to subtle bugs.
@@ -15,19 +18,23 @@

 non-uniform length.

 ## Example
+
 \```python
 zip(a, b)
 \```

 Use instead:
+
 \```python
 zip(a, b, strict=True)
 \```

 ## Fix safety
+
 This rule's fix is marked as unsafe for `zip` calls that contain
 `**kwargs`, as adding a `check` keyword argument to such a call may lead
 to a duplicate keyword argument error.

 ## References
+
 - [Python documentation: `zip`](https://docs.python.org/3/library/functions.html#zip)

So if we go the route of enforcing markdown style on rule docs it will require a lot of edits. Alternatively, if we only care about enforcing a line break around lists, this could be done with a simple python check. Something like:
if line followed by '-' or '*' then raise error

@augustelalande
Copy link
Contributor Author

Another option would be to format the docs as part of the generation process

@augustelalande
Copy link
Contributor Author

@zanieb what do you think between these three options? Remember the goal is just to ensure that lists get rendered properly.

  1. Enforce a strict markdown format with mdformat across the board. Requiring contributors to modify their docs before merging.
  2. Enforce a reduced markdown format with some custom python (namely line breaks around lists). Still requiring contributors to modify their docs before contributing.
  3. Automatically format docs with mdformat when rendering the website.

@zanieb
Copy link
Member

zanieb commented Mar 19, 2024

I honestly was imagining (3) but we'd probably want to run that check on pull requests too in case someone breaks the formatter in which case it would require manual intervention.

@augustelalande
Copy link
Contributor Author

What do you mean "breaks the formatter", like raise an unexpected error?

@zanieb
Copy link
Member

zanieb commented Mar 19, 2024

Yeah, idk if something goes wrong like it formats and can't render or the formatter errors.

@augustelalande
Copy link
Contributor Author

So adding it as part of generate_mkdocs.py would make sense, since that gets run for every PR.

@charliermarsh
Copy link
Member

So some of this is resolved, but I'm still seeing invalid references like this (possibly new ones?):

Screenshot 2024-03-20 at 8 30 42 PM

@augustelalande
Copy link
Contributor Author

Ya I was gonna work on that next.

The problem is that when someone attempts a reference to a setting, but doesn't make an options section with that setting, the reference doesn't get linked. My approach is gonna be to check any reference which matches the pattern [word][word], check if it's a valid setting, and if it is link it.

charliermarsh pushed a commit that referenced this issue Mar 21, 2024
#10484)

## Summary

The purpose of this change is mainly to address one of the issues
outlined in #10427. Namely, some lists in the docs were not rendering
properly when preceded by a text block without a newline character. This
PR adds `mdformat` as a final step to the rule documentation script, so
that any missing newlines will be added.

NB: The default behavior of `mdformat` is to escape markdown special
characters found in text such as `<`. This resulted in some misformatted
docs. To address this I implemented an ad-hoc mdformat plugin to
override the behavior. This may be considered a bit 'hacky', but I think
it's a good solution. Nevertheless, if someone has a better idea, let me
know.

## Test Plan

This change is hard to test systematically, however, I tried my best to
look at the before and after diffs to ensure no unwanted changes were
made to the docs.
@charliermarsh
Copy link
Member

Thanks, I fixed those specific docs here: #10498

@augustelalande
Copy link
Contributor Author

What are your thoughts on my comment though. Should we allow linking settings without an options section? If not should we flag attempts to do so?

@charliermarsh
Copy link
Member

Honestly, I'm fine with either. We should at least flag references that are missing from the Options section.

@augustelalande
Copy link
Contributor Author

Ok I will work on that

charliermarsh pushed a commit that referenced this issue Mar 21, 2024
## Summary
 
Some contributors have referenced settings in their documentation
without adding the settings to an options section, this has lead to some
rendering issues (#10427). This PR addresses this looking for potential
inline links to settings, cross-checking them with the options sections,
and then linking them anyway if they are not found.

Resolves #10427.

## Test Plan

Manually verified that the correct modifications were made and no docs
were broken.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants