SwiftGen / SwiftGen

The Swift code generator for your assets, storyboards, Localizable.strings, … — Get rid of all String-based APIs!
MIT License
9.24k stars 733 forks source link

Add parameter to filter (include/exclude) files #383

Closed djbe closed 5 years ago

djbe commented 6 years ago

Param to be able to filter files to parse in or out.

This would mitigate the problem about making ib scan XIBs and Storyboards for use cases when you only want storyboard scene constants and don't need to parse XIBs, but they would be parsed anyway and slightly impact performance on every build. Then storyboards would become an alias of ib --filter '*.storyboards'

The name of the argument is up for debate. --filter might be clearer than --exclude, because users might otherwise expect a --include parameter, which I don't know how we'd add.

djbe commented 6 years ago

I've looked into wath PathKit (and Foundation) supports, and it's not much. PathKit has a method for glob, but there's no globstar support (**), and the only thing I've found that supports it is: https://github.com/Bouke/Glob Problems with the latter are:

Instead, we might be better of only supporting regular expressions. If a user then wants to match (for example) all IB files in folders ending with "-iOS": --filter ".*-iOS\.(storyboard|xib)" It would be very easy even to allow for multiple filters, just check each path if it matches all regexes (internal or user provided).

AliSoftware commented 6 years ago

I'm ok with using RegExes instead of globs. Way easier to implement and wouldn't require another dependency.

Sure, it might not be as natural for users to write .*\.xcassets rather than just *.xcassets, that's true. But:

djbe commented 6 years ago

I did find this bit to convert glob patterns into regexes, the logic isn't that complicated (especially if we only support the "extended" part): https://github.com/fitzgen/glob-to-regexp But I'm not sure if it'd be worth the effort to support it (tests, maintain it, ...).

So my vote goes to regexes ✋.

AliSoftware commented 6 years ago

Nah, more risky and more to maintain for too little benefit imho†, so let's go with RegExes 👍

(† that's humble opinion for me, of course 😝 )

djbe commented 6 years ago

@AliSoftware seeing as #379 is ready to be merged (pending any remaining feedback), this will probably be the next item to be tackled. Not sure if it's a "requirement" for 6.0, we can always postpone this to 6.1.


Multiple filter types will exist:

How will we combine these? Do user filters override the built-in ones, or do we combine (AND) them? I think overriding will be simpler and at the same time allow the user more flexibility.

We should write out all the default filters here (and maybe also the command aliases), so we don't need to discuss these in the PR.

djbe commented 6 years ago

Filters:

Notes: currently we do the (font) matching based on the UTI type ("public.font"). I couldn't find any concrete list of supported extensions, there are some older ones such as dfont and other bizarre stuff. The user can always match those with a custom regex.

AliSoftware commented 6 years ago

1) Yes on user filters overriding the default ones. Especially imagine we forgot a compatible extension, AND-ing filters would not allow the user to add additional files to parse, only restrict them.

2) we don't support stringdict files (yet) so maybe we should postpone adding them in the default filter until we do?

3) For font files I remember forgetting some extensions back when the test was done on file extensions. That's why I switched to the UTI test iirc. Aren't there UTxxx functions available in the SDK to list known file extensions given an UTI though? Would allow us to cover them all?

4) aren't .nib files compiled XIBs? So they're not XML files we can parse are they?

AliSoftware commented 6 years ago

Once our Parser protocol supports filter and we define a default filter for each command, it would be nice and easy to print that default filter in the usage help btw

AliSoftware commented 6 years ago

PS : I want to release 6.0 ASAP. We know how we work and how little free time we have to work on SwiftGen between work and social life, is ready to have that "one last PR before the next release" end up staying in progress ("almost there!") For days then weeks and postpone too much.

I say we release early, release often. That's not really a breaking feature anyway, just an addition, so it can wait for 6.1 (even if 6.1 might come fast) so we don't postpone 6.0 anymore. There are a lot of features in master for which a lot of people have been waiting for too long already (especially the ready-to-merge JSON/YAML parser one)

djbe commented 6 years ago
  1. Yeah, more of a reference for when it's implemented, I'll remove it.

  2. Note that iOS only supports ttf/otf. I can't really find a "list" of extensions, except some articles such as https://www.macissues.com/2014/05/06/font-types-supported-in-os-x/, but even then some types are referred by name instead of extension. Apparently there are even "font suitcases" with no extension, so I guess we'll just have to make a decent "default", and the user can always override this.

  3. Good point. Was just typing from memory about actual types in Xcode. Although... nibs can be loaded or maybe converted (back) to xib?

print that default filter in the usage help

Good idea

release soon

Totally agree. This shouldn't be a 6.0 blocker.

djbe commented 5 years ago

Implemented by #570, released in SwiftGen 6.1.0 🎉

mortenholmgaard commented 5 years ago

@djbe I just used the new filter syntax base on readme and there seems to be a minor error i the example:

strings:
  inputs: Resources/Base.lproj
  filter: .+\.strings$
  outputs:
    - templateName: structured-swift4
      output: Generated/strings.swift

For me to get it working I needed a to add a slash first as the dot needed to be escaped.

And the example under Strings could also be updated to the above:

strings:
  inputs: /path/to/Localizable.strings
  outputs:
    templateName: structured-swift4
    output: Strings.swift
djbe commented 5 years ago

@mortenholmgaard You mention you needed to escape the dot, but the example in the readme already escapes the dot? And what's the difference between your (2nd) strings example and the one we provide?

AliSoftware commented 5 years ago

@djbe I didn't test, but my understanding of @mortenholmgaard 's comment is:

  1. He's telling he needed to escape the first dot (in addition to the second one). That might be a YAML thing and not a RegEx thing? If that's the issue, maybe putting the filter regex in quotes in the config file would help, and adding a note in the docs to suggest to always quote it would then be a good idea. We'll have to test that theory first of course.
  2. He's suggesting to replace the second strings example we provide in the README, and that he indeed copied verbatim in his comment, with the first one (with the filter). I don't necessarily agree as I prefer to keep the README simple and showing the most common usage, and using filter is an advanced usage that most people won't need to use given our default filters will generally suffice, but maybe his suggestion came from the fact that it's not clear enough in the documentation that filter is an optional parameter AND that we provide reasonable default filters for each command? In that regard, the example with strings specifying a filter that happens to be too similar to the default filter might not have been the best choice for the documentation, leading to that confusion?