-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fail loudly on invalid query params #4274
Comments
At an abstract level, I do agree with this. However, there are a few practical things to think about:
|
Agreed 👍
For a bad named logo, one option would be to display something in place of the logo. Not sure any of these are exactly the right thing, but it gives an idea: ⬜⛔💔😶🙈🕳️ Alternatively we could check in a list of formerly used icons. Going forward this could be automatically verified with a unit test. Then we could display something like 💨for deprecated icons and show an error message for completely made-up names. It's too bad we have named and svg logos in the same parameter; it makes it a little harder to know what the intention was when the parameter is neither. Though I suppose any encoded svg logo is going to be longer than a named logo.
Heh, yea, I can imagine that being a little bit annoying. Though at least there is some upside; when the error badge shows they'll know there is a problem and can fix their URL. I'm sure some people would prefer to just get the default color, though others might prefer to know they have a problem. We could help this a little by having a transition period when the badge shows with the default color with a little warning on it: And then on the website we could display a prominent message about it. |
📋 Description
In #4273 I argued that the Shields way is to fail loudly when parameters are incorrect, rather than silently ignoring them. Descriptive error messages point the user as directly as possible to the source of the problem rather than leaving them to wonder.
In particular, it seems like a good idea to return descriptive errors for:
cacheSeconds
which are too lowThe text was updated successfully, but these errors were encountered: