-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improvements to documentation #12712
Conversation
docs/requirements.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had an issue before updating the dependencies. Now these dependencies should be the same as in uv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any changes which requires the dependency upgrade? If not, I'd prefer to keep the upgrades separate as they could require some changes in the config / docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR primarily introduce extra js for the copy button.
Without upgrading dependencies, if you try to start the server, you will encounter a jinja2.exceptions.UndefinedError: 'mkdocs.config.config_options.ExtraScriptValue object' has no attribute 'endswith'
error.
This issue has been fixed in a newer version, as seen in the release notes: https://www.mkdocs.org/about/release-notes/#version-152-2023-08-02. Therefore, I believe it's best to also upgrade the dependencies to match the version used by uv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It seems that uv
's insider version is also fixed to mkdocs==1.5.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's mkdocs>=1.5.0
which would install 1.6.0
if possible: https://github.com/astral-sh/uv/blob/d0b16f90185ca7783d114306ade1626156ea6e29/docs/requirements.in#L2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but not for the insiders version? It uses the same commit as ruff. https://github.com/astral-sh/uv/blob/d0b16f90185ca7783d114306ade1626156ea6e29/docs/requirements-insiders.txt#L71-L76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I understand is that this issue was fixed in 1.5.2. I would prefer if we upgraded to the latest 1.5.x
version instead of bumping the minor as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I hope that it solves all regressions so that we can merge this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I don't have access to the insider version, I can't help much if regressions occur.
\cc @zanieb |
(I don't have access to mkdocs insider, so I don't change insider-related dependencies.) |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I think the updated dependencies are creating trouble in our documentation generation script (see CI).
Is it intentional that this PR updates markdown files that aren't hosted on the website?
README.md
Outdated
```shell | ||
# With pip. | ||
pip install ruff | ||
```console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change how github or pypi renders the readme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! This does change how it appears on github (and maybe pypi too), but readme on those platforms wouldn't benefit from the patch we introduced in this PR. So, the dollar sign will still be displayed, and we can't just ignore it when copying.
Therefore, we probably shouldn't include this change in the README.md
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I think we should only change the markdown files in the docs folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually it seems we only want to apply the changes to files in the crates/
or docs/
directories?
Yeah, we should just change files that are hosted on site.
I can look into the mkdocs failure once we answered all other questions. |
I clicked through the page and most things look okay after the upgrade but there are some regression that needs fixing before we can ship this. I would also like @charliermarsh or @zanieb to have a look at this. They'll know better why the insider version on uv is out of sync This is the
|
Yeah, I could also see this happening locally without using the insider version. And this likely didn't happen with |
docs/js/extra.js
Outdated
const start = performance.now(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be removed before merging, or is it intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops no. I'm sorry. That's absolutely not intentional
docs/js/extra.js
Outdated
@@ -37,6 +39,8 @@ function setCopyText() { | |||
); | |||
} | |||
}); | |||
|
|||
console.log(performance.now() - start); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
bb6f4e1
to
93e09a7
Compare
The trick is to not upgrade mkdocs-material |
Indeed! I also found it #12712 (comment). I love the improvement you've introduced in the latest commit. 👍 |
Summary
As we did in astral-sh/uv#5397, we improve the copy behavior for documentation in this PR.