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
Add svg linter to check center (with tolerance) #3250
Add svg linter to check center (with tolerance) #3250
Conversation
Whoa! Nice, quick PR, @davidjb 🙂 And there I thought it might require some further discussion in the original PR first! 😆 Before we get to reviewing this properly, out of interest, how many SVGs fail the test if you do allow for a variance of |
Nice work once again @davidjb 👌 Regarding ignoring these errors, I would suggest keeping it separate from the other list, but somehow include it in the same file (e.g. an array with two objects)
Seems like that would be 51 with the existing list of ignored SVGs and 127 in total. |
I think we should allow for that variance, so as it could well be caused by rounding in icons with a precision greater than 3. And, of course, creates less work for us in fixing it! 😆
I'd agree with that. I'd also suggest generating a list of just the 51 icons not also caught by #3107 and adding them to #3169 for fixing, work through it all as one. |
I can add the above suggestions in. Before I revise the PR, following the discussion at #3253, is there a desire to add the same or similar level of tolerance to the size linter as well? e.g to accept dimensions ranging from 23.999 to 24.001. My feeling is that if people are editing or updating icons then getting it exactly right isn't much more effort, at least no more so than my experience grappling with rounding caused by |
I would personally keep them separate, though that might get us in some confusing situations when we don't cross an icon off both lists 🤔 In that regard I'm fine either way, and I would merge all three of the lists into one.
That, to me, seems like a separate discussion that can be handled in a separate PR. Could you update this PR with the above suggestions @davidjb? For now, I haven't considered it very well but it seems sensible to include a similar level of tolerance in the size linter but @PeterShaggyNoble disagrees. |
Okay so just to confirm the changes I need to make are:
Just checking before I get started. |
Correct @davidjb 👍 |
All done and pushed. The PR now has tolerance for at most +/- 0.001 in any direction. The ignore file was updated and restructured (I used the code at #3107 (comment) to dump again) -- 81 additional icons were ignored because of the centring linter. The size linter only checks against the I thought of this a while ago with the ignore list - potentially adding an ignore reporter like this into the svglintrc, maybe as a disabled-by-default, separate linter? That way, project admins or enthusiastic contributors can easily discover which icons were ignored and why. I would say enable the warnings by default since warnings won't break a CI build - but the 369-odd files on the ignore list generate a lot of output that'd probably spook a contributor. Anyway, here's the code as an example: if (iconIgnored.size.hasOwnProperty(iconPath)) {
reporter.warn(`Icon was ignored due to size linter`);
return
} else if (iconIgnored.center.hasOwnProperty(iconPath)) {
reporter.warn(`Icon was ignored due to center linter`);
return
} |
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.
This looks good to me 👍 since it's a pretty important change I would like to have at least one other @simple-icons/maintainers look at this before merging.
Regarding the warnings: it's a nice suggestion but personally I feel like it becomes annoying when there actually is an error. In that case it is burred between over 300 warnings that aren't really relevant. This, as you say, may spook some people away from contributing.
I'd be okay with variants where e.g. they only show in the CI or if you supply a special argument upon invocation.
Whether we add warnings or not, I'd suggest we leave that for a different PR and get this linting check merged as quickly as possible 😃
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.
Overall I'm glad for this nice addition! 👍
God job!
This adds a tolerance of +/- 0.001 to either the X or Y dimensions, adjusting the existing ignore list and size linter to fit the new structure of the ignore file. See simple-icons#3107 (comment)
As above, I've added @runxel's suggestion of |
Thanks again for this very useful contribution @davidjb, we all appreciate it very much! 🎉🎉 @PeterShaggyNoble will you look into updating (and merging?) the list(s) of icons that are being ignored? |
You’re welcome! I feel bad as I keep making more work for people. Is there anything else that needs linting? 😅 |
Thanks for another valuable contributions, @davidjb 👍
Feel free to pitch in on cleaning it all up! 😆
On it! |
@davidjb, looks like something went wrong when generating the values for the list of icons to ignore; only the first word in each brand name was included. |
Hmm very curious. I see what happened, my title extraction code was incredibly naïve, assuming no spaces in the brand name. Shall I get on it and send an update PR with regenerated/updated ignore lists? |
👍 Yes please, @davidjb |
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
All done @PeterShaggyNoble, see the new PR over at #3433. To allow automated updating of the ignore file much easier, the code's now embedded in the svglintrc; that'll help with any future linters that might come along too. |
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set and the linter is run like so `SI_UPDATE_IGNORE=true npm run svglint` -- running this command will display all remaining linting failures and use these to update the ignore file. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set and the linter is run like so `SI_UPDATE_IGNORE=true npm run svglint` -- running this command will display all remaining linting failures and use these to update the ignore file. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per #3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set and the linter is run like so `SI_UPDATE_IGNORE=true npm run svglint` -- running this command will display all remaining linting failures and use these to update the ignore file. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set and the linter is run like so `SI_UPDATE_IGNORE=true npm run svglint` -- running this command will display all remaining linting failures and use these to update the ignore file. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
This updates the ignore list as per simple-icons#3250 and embeds code for updating the ignore file when the environment variable `SI_UPDATE_IGNORE=true` is set and the linter is run like so `SI_UPDATE_IGNORE=true npm run svglint` -- running this command will display all remaining linting failures and use these to update the ignore file. This also: * Normalises the top-level object key in the ignore file to match each linter name * Sorts the ignore output on top-level key and icon name value to make output consistent * Speeds up icon center checking if ignored * Ignores icons in size linter if icon is otherwise ignored
Issue: #3107 (comment)
Description
This adds a custom linter to svglint in the same vein as the last one #3107 to pull out the path, run it through a bounding box checker, calculate the centre X/Y of the path and test that the result is 12,12 (being the centre of 24,24). The math precision is to 3 decimal places, configured from the same config variable as last time, to avoid rounding errors in floating point comparisons. Whether some additional tolerance should be added to say accept values between 11.999 and 12.001 is up for discussion...
In this case, adding this linter with this precision fails on 461 files..so I suppose another ignore list is in order given the additional fixes required. 😅 Hence the WIP title to open this to discussion. I can add an ignore in the exact same way as before, just with a different filename so you can track which lint failures are being ignored, or simply add them into the existing ignore file and any future change in path (eg fixing the icon size) will knock it off the ignore list anyway...
Example lint output (ignore list bypassed):
Some are very close and some are quite a ways off (relatively speaking when considering 3 decimal points..!), in either dimension.
461 more icons to fix 😰 - don't shoot the messenger 😎. With the existing ignore list in place, it's showing as 244 invalids at time of writing.