Add svg linter to check center (with tolerance) #3250

davidjb · 2020-06-23T13:57:49Z

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):

[...]
x icons/webpack.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 11.971 x 12.357
x icons/radiopublic.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.002
x icons/wikipedia.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.001
x icons/wikimediacommons.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12.001 x 12
x icons/windows95.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12.001 x 12
x icons/wolframlanguage.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.001
x icons/write-dot-as.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12.003 x 12
x icons/x-dot-org.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 11.999
x icons/xamarin.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.001
x icons/xcode.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 11.933 x 12.031
x icons/xdadevelopers.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 11.999 x 12
x icons/xmpp.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.001
x icons/youtube.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 11.999
x icons/zalando.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 11.509 x 12
x icons/zdf.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 11.998
x icons/zillow.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.001
x icons/zoom.svg
  x icon-centered <path> must be centered on the canvas; the center is currently 12 x 12.024

------------ Summary ------------
✓ 909 valid files.
x 461 invalid files.

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.

PeterShaggyNoble · 2020-06-23T14:36:28Z

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 .001?

ericcornelissen · 2020-06-23T15:16:25Z

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)

Before we get to reviewing this properly, out of interest, how many SVGs fail the test if you do allow for a variance of .001?

Seems like that would be 51 with the existing list of ignored SVGs and 127 in total.

PeterShaggyNoble · 2020-06-25T09:39:21Z

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! 😆

Regarding ignoring these errors, I would suggest keeping it separate from the other list, but somehow include it in the same file

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.

EDIT: Should we merge #3120 with #3169 while we're at it?

davidjb · 2020-06-26T00:24:21Z

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 svgo. That said, I'm not the one doing the editing. I suppose certain icons may be more difficult than others to ensure they're correct so it's up to the project to decide what tolerance is acceptable and in what way.

ericcornelissen · 2020-07-05T07:57:58Z

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.

EDIT: Should we merge #3120 with #3169 while we're at it?

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.

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.

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.

davidjb · 2020-07-06T23:37:22Z

Okay so just to confirm the changes I need to make are:

adding a tolerance of +/- 0.001 to the centre-checking linter, and
adding the ignore list for the ~51 icons that fail this new test (which will involve restructuring the ignore file in the process)

Just checking before I get started.

ericcornelissen · 2020-07-08T19:57:47Z

Correct @davidjb 👍

davidjb · 2020-07-08T23:57:47Z

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 size ignore list but the center linter checks against both.

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
            }

ericcornelissen

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 😃

runxel

Overall I'm glad for this nice addition! 👍
God job!

.svglintrc.js

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)

davidjb · 2020-07-09T23:26:40Z

As above, I've added @runxel's suggestion of Math.abs (thanks!) and modified the reporter output to explain what the centre should be and what it currently is.

ericcornelissen · 2020-07-10T08:07:51Z

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?

davidjb · 2020-07-10T08:24:31Z

You’re welcome! I feel bad as I keep making more work for people. Is there anything else that needs linting? 😅

PeterShaggyNoble · 2020-07-10T13:57:30Z

Thanks for another valuable contributions, @davidjb 👍

I feel bad as I keep making more work for people.

Feel free to pitch in on cleaning it all up! 😆

will you look into updating (and merging?) the list(s) of icons that are being ignored?

On it!

PeterShaggyNoble · 2020-07-10T14:14:36Z

@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.

davidjb · 2020-07-10T22:05:31Z

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?

PeterShaggyNoble · 2020-07-24T14:48:20Z

👍 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

davidjb · 2020-08-08T06:58:31Z

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 * 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

davidjb requested a review from PeterShaggyNoble June 23, 2020 13:58

davidjb mentioned this pull request Jun 23, 2020

Add linter to check svg path dimensions #3107

Merged

3 tasks

PeterShaggyNoble added in discussion There is an ongoing discussion that should be finished before we can continue meta Issues or pull requests regarding the project or repository itself labels Jun 23, 2020

PeterShaggyNoble requested review from ericcornelissen, NovaGL and runxel June 23, 2020 14:39

PeterShaggyNoble mentioned this pull request Jun 25, 2020

The Great 2020 Icon Review #3169

Closed

davidjb mentioned this pull request Jun 25, 2020

Add Photobucket #3253

Merged

3 tasks

davidjb force-pushed the lint-svg-centre branch from 12ccddb to e841a31 Compare July 8, 2020 23:55

ericcornelissen approved these changes Jul 9, 2020

View reviewed changes

ericcornelissen removed the in discussion There is an ongoing discussion that should be finished before we can continue label Jul 9, 2020

davidjb changed the title ~~WIP: Add svg linter to check center~~ Add svg linter to check center (with tolerance) Jul 9, 2020

runxel reviewed Jul 9, 2020

View reviewed changes

.svglintrc.js Outdated Show resolved Hide resolved

.svglintrc.js Outdated Show resolved Hide resolved

davidjb force-pushed the lint-svg-centre branch from e841a31 to dbc5a32 Compare July 9, 2020 22:48

Add svg linter to check center

ac3fdd1

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)

davidjb force-pushed the lint-svg-centre branch from dbc5a32 to ac3fdd1 Compare July 9, 2020 23:14

Merge branch 'develop' into lint-svg-centre

3303744

ericcornelissen merged commit cc509d9 into simple-icons:develop Jul 10, 2020

PeterShaggyNoble mentioned this pull request Jul 10, 2020

Add Drooble icon #3231

Merged

3 tasks

davidjb mentioned this pull request Aug 8, 2020

Update svglint ignore list, add ignore list updater #3433

Merged

3 tasks

mondeja mentioned this pull request Nov 22, 2020

Replace 'svg-path-bounding-box' with 'svg-path-bbox'. #3977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add svg linter to check center (with tolerance) #3250

Add svg linter to check center (with tolerance) #3250

davidjb commented Jun 23, 2020 •

edited

PeterShaggyNoble commented Jun 23, 2020

ericcornelissen commented Jun 23, 2020

PeterShaggyNoble commented Jun 25, 2020 •

edited

davidjb commented Jun 26, 2020

ericcornelissen commented Jul 5, 2020

davidjb commented Jul 6, 2020

ericcornelissen commented Jul 8, 2020

davidjb commented Jul 8, 2020

ericcornelissen left a comment

runxel left a comment

davidjb commented Jul 9, 2020

ericcornelissen commented Jul 10, 2020

davidjb commented Jul 10, 2020

PeterShaggyNoble commented Jul 10, 2020

PeterShaggyNoble commented Jul 10, 2020

davidjb commented Jul 10, 2020 •

edited

PeterShaggyNoble commented Jul 24, 2020

davidjb commented Aug 8, 2020

Add svg linter to check center (with tolerance) #3250

Add svg linter to check center (with tolerance) #3250

Conversation

davidjb commented Jun 23, 2020 • edited

Description

PeterShaggyNoble commented Jun 23, 2020

ericcornelissen commented Jun 23, 2020

PeterShaggyNoble commented Jun 25, 2020 • edited

davidjb commented Jun 26, 2020

ericcornelissen commented Jul 5, 2020

davidjb commented Jul 6, 2020

ericcornelissen commented Jul 8, 2020

davidjb commented Jul 8, 2020

ericcornelissen left a comment

Choose a reason for hiding this comment

runxel left a comment

Choose a reason for hiding this comment

davidjb commented Jul 9, 2020

ericcornelissen commented Jul 10, 2020

davidjb commented Jul 10, 2020

PeterShaggyNoble commented Jul 10, 2020

PeterShaggyNoble commented Jul 10, 2020

davidjb commented Jul 10, 2020 • edited

PeterShaggyNoble commented Jul 24, 2020

davidjb commented Aug 8, 2020

davidjb commented Jun 23, 2020 •

edited

PeterShaggyNoble commented Jun 25, 2020 •

edited

davidjb commented Jul 10, 2020 •

edited