-
Notifications
You must be signed in to change notification settings - Fork 21
Ignore style guide ids #46
base: main
Are you sure you want to change the base?
Conversation
Hey @LucaGobbo |
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.
I'm still trying to figure out if all the details are ok but i gave this a quick review.
Also, this needs more tests. Specifically that this configuration is actually respected. Perhaps in the TemplateParser level, while using a config.
Sources/PrismCore/Prism.swift
Outdated
|
||
errors.append(contentsOf: styleguideErrors) | ||
|
||
// Get text styles, colors and spacing separately | ||
// for each styleguide | ||
|
||
let styleguideIDs = unfilteredStyleguideIDs.filter { id in !ignoredStyleGuideIds.contains(id) } |
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.
Change to two Sets and symmetric difference, should be more efficient
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.
@freak4pc When I change this to your suggestion, for some reason this unit test fails, do you think it has to do with the incorrect order
this was the code I used Set(allStyleguideIds).symmetricDifference(Set(ignoredStyleGuides))
failure:
failed - expected to equal <Duplicate colors found: green, mud, purple, tealDuplicate text styles found: Base Heading, Body, HighlightDuplicate spacings found: spacing-m, spacing-xl, spacing-xs, spacing-xxs>,
got <Duplicate colors found: green, purple, tealDuplicate text styles found: Base Heading, Body, HighlightDuplicate spacings found: spacing-m, spacing-xl, spacing-xs>
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.
Doesn't seem to be sorting or order related,
If you break the message you'll notice there's "mud" missing and "spacing-xxs"
Duplicate colors found: green, mud, purple, teal
Duplicate text styles found: Base Heading, Body, Highlight
Duplicate spacings found: spacing-m, spacing-xl, spacing-xs, spacing-xxs
Duplicate colors found: green, purple, teal
Duplicate text styles found: Base Heading, Body, Highlight
Duplicate spacings found: spacing-m, spacing-xl, spacing-xs>
I can dive into the tests a bit later but not sure this is expected.
Codecov Report
@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 97.33% 97.35% +0.01%
==========================================
Files 21 21
Lines 1578 1586 +8
==========================================
+ Hits 1536 1544 +8
Misses 42 42
Continue to review full report at Codecov.
|
So sorry for taking so long to review this! @LucaGobbo Would you mind renaming it to just I'm just wondering if we should encapsulate it differently in the configuration, but we can possibly do that later on if the need arises. Thank you! |
With this new configuration option we can specify which style guide id's we want to ignore
Co-authored-by: Shai Mishali <freak4pc@gmail.com>
Co-authored-by: Shai Mishali <freak4pc@gmail.com>
c7b8370
to
a1d5fec
Compare
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! Sorry this took so long.
Hi,
We want to be able to not fetch certain style guides, in our workspace we use the same naming in multiple style guides. Because of this, we want to be able to skip the fetching of certain style guides.
To accomplish this I've added the option to the configuration, so this is now possible. Hopefully, this can be added to the project
Thanks!