Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Ignore style guide ids #46

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

LucaGobbo
Copy link

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!

@freak4pc
Copy link
Contributor

freak4pc commented Nov 4, 2020

Hey @LucaGobbo
Thanks for the great PR! Next time it's best to open an issue so we can discuss before implementation, let me think about this some more, I wouldn't want tens of flags to configure every tiny aspect, but this seems mostly reasonable

Copy link
Contributor

@freak4pc freak4pc left a 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/Models/Configuration.swift Outdated Show resolved Hide resolved
Sources/PrismCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Sources/PrismCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Sources/PrismCore/Prism.swift Outdated Show resolved Hide resolved

errors.append(contentsOf: styleguideErrors)

// Get text styles, colors and spacing separately
// for each styleguide

let styleguideIDs = unfilteredStyleguideIDs.filter { id in !ignoredStyleGuideIds.contains(id) }
Copy link
Contributor

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

Copy link
Author

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>

Copy link
Contributor

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
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #46 (90487fb) into main (6ad1d4f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
Sources/PrismCore/Models/Configuration.swift 100.00% <100.00%> (ø)
Sources/PrismCore/Prism.swift 86.90% <100.00%> (+0.07%) ⬆️
Tests/ConfigurationSpec.swift 100.00% <100.00%> (ø)
Tests/TemplateParserSpec.swift 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ad1d4f...90487fb. Read the comment docs.

@freak4pc
Copy link
Contributor

So sorry for taking so long to review this! @LucaGobbo
Given a rebase and fixing up the CI, I think we can merge this.

Would you mind renaming it to just ignored_styleguides without the specificity of ID?

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!

Luca Gobbo and others added 7 commits January 14, 2021 10:18
Copy link
Contributor

@freak4pc freak4pc left a 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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants