Mark the now-done [cmp.result] in spaceship projects as complete; normalize some status markers for papers and projects; fix alignment and line breaks in spaceship projects, add links to standard
Details
- Reviewers
cjdb Mordante mumbleskates - Group Reviewers
Restricted Project - Commits
- rG3fe7dde5f1a3: [libc++][doc] Cleanup, normalize, and update projects status docs
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I'm not too fond of patches doing multiple things in one review. It makes finding the real changes a lot harder.
Mark one project in spaceship completed;
Finding this which project that is is quite hard.
I also see some files marked as modified, but without obvious changes.
libcxx/docs/Status/Cxx14.rst | ||
---|---|---|
1–52 | What has changed in this file? | |
libcxx/docs/Status/FormatIssues.csv | ||
3 | Please don't change the status of these entries. There's a patch in review. Unlike the Status/Cxx*.rst pages these are really intended for the developers working on these features. Changing the status makes it harder for me to keep track of the real status. | |
libcxx/docs/Status/RangesPaper.csv | ||
8 ↗ | (On Diff #367936) | Why change this? I'm not working on Ranges so don't have a strong opinion. But I assume the authors picked the checkmark since they prefer it that way. |
libcxx/docs/Status/SpaceshipProjects.csv | ||
2–8 | Thanks for adding these section links! |
The specific project that was completed was [cmp.result], the only thing on the list that is marked as complete so far. I can change the differential description to name it specifically. In my head this easily falls into the same bucket as cleaning up formatting, as it is correcting trivial information that is currently wrong.
libcxx/docs/Status/Cxx14.rst | ||
---|---|---|
1–52 | Line endings were CRLF, out of line with other files, for Cxx14-2b and Styles; I can revert all these if that's preferable. Notably this does not affect git blame. | |
libcxx/docs/Status/FormatIssues.csv | ||
3 | Ack | |
libcxx/docs/Status/RangesPaper.csv | ||
8 ↗ | (On Diff #367936) | Accessibility; consistency; ability to enter it in without hunting down another check mark and copy-pasting it. Assuming we want to keep the check marks for some reason it's better to create an entry in Styles.rst like .. |checkmark| unicode:: U+2705 and use that. This is mostly cjdb's domain, I'll leave it up to him whether he prefers text or a check mark. |
I'm with @Mordante on splitting this up into two patches, but I can see why you've combined them.
libcxx/docs/Status/FormatPaper.csv | ||
---|---|---|
1–2 | It'd be great if you could inject Not started and Unassigned into everything that's blank. | |
libcxx/docs/Status/RangesPaper.csv | ||
2 ↗ | (On Diff #367973) | Please leave this as Sentence case (similarly elsewhere). Alternatively: abandon all changes to this file, as it'll probably get deleted once P0896 is fully implemented. This doc exists mainly to help @zoecarver, @ldionne, and me coordinate who's doing what, and I'm okay with it being a bit different. |
8 ↗ | (On Diff #367936) | No strong preference, as copy/pasting a checkmark is pretty trivial now that it's being used in this file. |
- revert RangesPaper.csv
- Another pass at styling Format statuses
libcxx/docs/Status/FormatPaper.csv | ||
---|---|---|
1–2 | I added unassigned, |Not Started|, and a styled replacement for |Review|. And some more section links! | |
libcxx/docs/Status/RangesPaper.csv | ||
2 ↗ | (On Diff #367973) | haha, i was keeping these titlecased because all the existing examples are titlecased, not because i prefer it. |
8 ↗ | (On Diff #367936) | yeah, we have the whole green background thing going on on the actual page now though which is nice. the check mark on the other hand barely shows up in some of my text editors; i really prefer ascii source files when it's achievable. given your expressed preference against titlecased statuses or whatever i think i'll just revert this file entirely since nothing was added to it other than normalizing the style. |
libcxx/docs/Status/Cxx14.rst | ||
---|---|---|
1–52 | I don't object against the change, but it was unclear what was changed and it wasn't mentioned in the patch description. I think it would make sense commit this separately, maybe split the patch after review. | |
libcxx/docs/Status/FormatIssues.csv | ||
14 | Since these are on the same format page as the file below. I think here we should also add |Not Started| for consistency. | |
libcxx/docs/Status/FormatPaper.csv | ||
12 | I like adding the wg21 links, but if we add them we should do it consistently in this document. |
- updates from feedback
libcxx/docs/Status/Cxx14.rst | ||
---|---|---|
1–52 | It is mentioned in the patch description. Either way is probably fine. As it is this patch is general docs cleanup, and these are the only 5 files with CRLF in all of libcxx. | |
libcxx/docs/Status/FormatPaper.csv | ||
12 | I just added it to the first occurrence of every section. I'll add it to every header if that's what you want. |
LGTM, I still prefer to commit it in separate patches.
libcxx/docs/Status/FormatPaper.csv | ||
---|---|---|
12 | Ah I didn't notice that. Then for me the current way is fine. |
If you want me to split this up more somehow I can do that too, but it does become increasingly more work for everyone involved.
Rebase onto main. I'll commit this, I reviewed the changes and they look OK to me.
Indeed, it's generally good to try to split functional and non-functional changes from
the start to ease the review process, especially as one is building trust with the reviewers.
But this is an improvement for sure, so I'm shipping this now.
What has changed in this file?