[#4429] Fix Error Retrieving Old Config Options #4451
Conversation
|
Thank you for the PR @jdollar. The ability to dynamically show-version specific configuration information on the site is a pretty new feature, and historically the site only showed the available config opts based on what was in the rustfmt source. As such, I'd rather we just remove invalid versions from the available list, especially considering how ancient 0.8.1 and 0.7 are. The purpose of the site is to provide detailed information about the configuration options, so there's really no point in having special cases and extra code for these old edge cases where that core feature can't be met. Would you be interested in updating this to remove those two from the list? |
|
@calebcartwright For sure. Sounds good to me. I'll make that update to just get rid of the two ancient versions. |
Older tags of the repo don't have the configuration.md file that the docs/index.html file uses to display configuration options. Removing them from the list since they don't apply to the use case of the documentation page.
jdollar
force-pushed
the
4429_fix_older_version_configuration_display
branch
from
1926a21 to
b3d6fa5
Compare
|
It is...a little more tame now. 😉 Just added a check on the filter condition that populates the variable used in the dropdown. Additionally I removed that |
| @@ -181,10 +181,12 @@ | |||
| this.handleReqFailure(e); | |||
| return; | |||
| } | |||
| const reMajorVersion = /v(\d+)/; | |||
|
|
|||
| const excludedTagVersions = new Set(['v0.7', 'v0.8.1']); | |||
There was a problem hiding this comment.
Thanks for the update, this is indeed a much simpler diff 😆 I don't recall off hand what the browser compatibility breakdown is for Set's, although for that matter I don't know if there's any potential compatibility issues elsewhere within this page.
Do you know what the compatibility story is for Set.has? I suppose we could fallback to an array with an includes or indexOf if necessary, or even and object with key checks.
These are both really small data collections so the browser compatibility weighs more heavily than performance and/or ergonomic gains of newer constructs
There was a problem hiding this comment.
I was using the MDN compatibility chart for determining if there were any issues with utilizing the set and set.has functionality: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set
It seemed pretty widely adopted so I went with it, but I'm totally open with using the array functions if we wanted to go that route. I guess like, what were we hoping to achieve with compatibility with the page? What are the thresholds we are shooting for with browser versions getting supported?
There was a problem hiding this comment.
I guess like, what were we hoping to achieve with compatibility with the page? What are the thresholds we are shooting for with browser versions getting supported?
Great question, and to be perfectly honest it's completely undefined to the best of my knowledge. The fact that this will even work on IE (albeit 11) is more than good enough for me.
|
Thank you @jdollar! The frontend config site could probably use a little attention if you're interested (and plenty of work elsewhere within rustfmt too) |
|
backported in #4958 |
Add code to the doc/index.html file to handle the specific cases for version 0.8.1 and 0.7
of rustfmt where they don't have a explicit configuration.md file. Updated to pull the data
that is within the "Configuring Rustfmt" section of the original README