Rustdoc migrate to table so the gui can handle >2k constants by dns2utf8 · Pu...
source link: https://github.com/rust-lang/rust/pull/88816
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
r? @ollie27
(rust-highfive has picked a reviewer for you, use r? to override)
Might be worth it to add a check on test and/or rustdoc hash version to check if we need to rebuild docs or not, even more with this change...
Or maybe we could just create this file once and for all. Not sure what's the best solution here. An opinion @Mark-Simulacrum ?
I marked this PR as blocked until #88776 is merged.
Might be worth it to add a check on test and/or rustdoc hash version to check if we need to rebuild docs or not, even more with this change...
Do you mean that we only run the tests that have failed or are new? Or about (re-)generating the HTML and such?
Or maybe we could just create this file once and for all. Not sure what's the best solution here. An opinion @Mark-Simulacrum ?
We can store the generated file too, sure
This comment has been hidden.
This comment has been hidden.
Oh wow, that's awesome! So just remains to generate a 2000 line file with the constants I guess for the GUI test? :)
Once done, let's merge it, great work, thanks!
Please don't check in a 2000-line generated file, we can generate it at test runtime quickly enough that it shouldn't be necessary to pregenerate and bloat everyone's repository unnecessarily.
the grid-2 spec talks about -10k to +10k cells, it is currently a blink-engine thing to clamp at 1k. In some of their examples I saw them using 1.5k lines so I guessed 2k.
I'd be interested in hearing more about our motivation for adding this test, though. Based on what's been said so far, it sounds like the test is intended to avoid us regressing back to using grids specifically for this feature -- but that seems like the wrong thing to target. In practice, if grids are problematic with large tables -- whether the clamp is at 1000 or 10,000 doesn't seem like it matters that much in practice - then it seems like the better approach is to add a tidy lint preventing their usage in our css files. Obviously, if we know ahead of time that the lint doesn't apply to some statically known small-size user, then we can avoid it in that case. But this would both save on the generation of a 2k line file and the related annoyances (e.g., test run time, etc.) and target the problem more holistically, right?
@Mark-Simulacrum Then we need to improve how we run GUI tests because currently we regenerate test docs every time whether or not something was changed. I can do that in a follow-up PR.
Also, @dns2utf8 removed the grid and replaced it with something else, completely fixing the issue.
@Mark-Simulacrum Then we need to improve how we run GUI tests because currently we regenerate test docs every time whether or not something was changed. I can do that in a follow-up PR.
This seems to have nothing to do with my request? I expect each test to either run from scratch or not at all. We can add some caching atop that similar to compiletest, though.
Also, @dns2utf8 removed the grid and replaced it with something else, completely fixing the issue.
Yes, I gathered that. What I'm saying is that testing that we can support 2,000 constants doesn't seem necessary, we can test directly for grid usage and lint against that.
Yes, I gathered that. What I'm saying is that testing that we can support 2,000 constants doesn't seem necessary, we can test directly for grid usage and lint against that.
Then I don't understand what you mean.
Currently, there is no use of grid layouts in rustdoc and adding a lint that prevents their use could work too for now.
If we decide to use a grid in the future for something else (like switching the layout between super large desktops and 1080p-like desktops) then the check might get removed again. Adding a comment to the lint could reduce that risk again.
Wild idea (maybe a bad one): what if we move some of the tests into a separate folder that is only run at release?
No, we don't want a special category of tests, those just cause pain all around.
Yes, the lint would prevent usage of grids by accident; the reviewer/author could of course justify the usage in specific cases and silence the lint on that particular usage. Tidy lints don't just get dropped, so this would be a reliable way of ensuring we don't make this mistake in the future at very low overhead.
I'm very curious on how you want to write such a lint considering all the debate around #86178 (which is still open)...
Oh right, I had in mind potential adds through inline CSS (very unlikely) and through JS. Wel, if we don't consider those two, it should be easy enough.
A CSS lint seems reasonable.
If we want to keep the long check: what if we reexport some huge module (like one with the arm platform constants) in the test docs and use that for checking?
Or would you just remove this test for now because the table style is very stable I think?
Commit bca503f has been approved by GuillaumeGomez
@bors r=GuillaumeGomez
Squashed the commits down a little.
Commit e599e2d has been approved by GuillaumeGomez
Thank you @Mark-Simulacrum for rebasing it! 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK