-
Notifications
You must be signed in to change notification settings - Fork 533
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
comment out FIXMEs to not display them on UI #2186
Conversation
bbf056b
to
b2933b8
Compare
In some cases the FIXME's are themselves information, some extra bit of nuance that the page doesn't cover that the reader should probably know exists even if we haven't actually talked about it yet. Or potentially it indicates they should take the page with a bit of salt as its not been updated in a while or is confusing as written |
FIXME: it is likely that this will subtly change again by mostly moving structural | ||
normalization into `NormalizesTo`. | ||
--> |
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.
normalization has changed pretty significantly since this chapter was first written iirc, imo we should have a big note at the top of the page saying that things have likely diverged from what is documented here.
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.
Sure, done that.
src/solve/opaque-types.md
Outdated
<!-- | ||
[^1]: FIXME: this should ideally only result in a unique candidate given that we require the args to be placeholders and regions are always inference vars | ||
[^2]: FIXME: why do we check whether the expected type is rigid for this. | ||
--> |
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.
There are footnotes linking to this which should also be removed if we're going to remove this.
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.
In that case they should stay as they were. I didn’t realize the links above.
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
b2933b8
to
db5d166
Compare
Yeah I agree on that, I left some of them visible but I wasn't entirely sure about others, that's why I wanted for a help in the PR description. |
@@ -2,7 +2,9 @@ | |||
|
|||
<!-- toc --> | |||
|
|||
> **FIXME(jieyouxu)** completely revise this chapter. | |||
<!-- | |||
FIXME(jieyouxu) completely revise this chapter. |
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.
FTR, this was intentional and I think it's more accurate to display this. But maybe elaborate with:
- This chapter is quite outdated at various places.
- The organization is confusing.
- It's potentially better to have per-directive doc comments then backlink to them for details, instead of having them separately listed here.
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.
It even includes a specific username to be resolved from. Why should it appear on the UI? Maybe I don't understand, what value does it bring to that page?
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.
Makes sense, I opened #2187 to track this instead.
For dev-guide specifically, it's not stable or user-facing, but more like internal contributor docs. IMO having contributor-facing FIXMEs are actually good because it conveys info about the page (esp. if it indicates the page is outdated or needs to be changed in some way). |
I honestly think that some FIXMEs shouldn't appear in the UI as they don't really make sense at all... Also, those pages often visited by newbies not just for internal use. |
Makes sense, we should probably convert those FIXMEs to actual issues in this repo anyway. |
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.
Thanks!
Prevents FIXMEs from appearing on the UI. Unless I am missing something, displaying these to readers doesn't make much sense. Some of them might make sense to include on the UI, but I am unsure which ones. If you think any should be displayed please mark them specifically.
This is how it currently looks:
And this is how it looks after this patch: