-
Notifications
You must be signed in to change notification settings - Fork 664
Add --regex-rules option (RFC)
#2280
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
base: develop
Are you sure you want to change the base?
Conversation
1dceb7c to
5a1e018
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2280 +/- ##
===========================================
- Coverage 95.29% 95.29% -0.01%
===========================================
Files 158 159 +1
Lines 23767 23835 +68
===========================================
+ Hits 22649 22713 +64
- Misses 1118 1122 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice! This is cool. I think it definitely makes sense to offer. We have 20-30 SwiftLint regex rules. Most of them are patterns that we just want to disallow. A couple of them are iOS APIs that we have an internal-only replacement for that is mostly API-compatible. Those examples would be nice to autocorrect with a SwiftFormat autocorrect regex instead of a lint-only rule. However like the Even though there are a lot of limitations, seems worth having. |
|
Config could be a bit easier organize / read the config file if each rule had a separate |
|
If the regex rule had the ability to add new imports, it cover more of the use cases we have. Could be something like: --regex-rule urlMacro/URL\(string: ("[^\\]+")\)!/#URL($1)/import URLFoundation |
As with all the comma-delimited options, this just works out of the box. If you pass the param multiple times then the results are concatenated. (Probably this should be made clearer in the docs somehow): |
Nice idea, but this syntax really doesn't seem scalable. I feel like maybe it's time to consider moving to yaml or something? I like that it's possible to provide all the config on the command line though, and I'm a bit reluctant to lose that. I wonder if there's some more readable way to pack arbitrary nested key/value pairs into a single option? |
|
Could the option take JSON content? That's more flexible than YAML since it isn't whitespace sensitive. You could put the JSON entirely on one line as needed e.g. over the command line, or if you prefer that organization in your config file. Could we trivially support both of: and: |
|
Or we could accept both JSON and YAML. JSON feels necessary for the command line use case. For YAML it could look like: I like the feel of JSON more here though. The balanced parens from the JSON feels nice and is going to be a lot easier to parse as well. I guess we could wrap the YAML in braces: |
ba32553 to
b1d6cff
Compare
5a1e018 to
9d06259
Compare
This PR adds a new option
--regex-rulesthat allows you to specify simple text replacement rules using regular expression syntax, as follows:The example above would (mostly) replicate the existing
urlMacrorule, making it possible for such custom rules to be implemented without needing to modify the library. This replicates a similar feature already available in SwiftLint.Since it operates purely at the string level, it arguably doesn't need to be part of SwiftFormat at all, however it is able to advantage of SwiftFormat's file matching, caching and and parallel rule processing, as well as respecting
// swiftformat:disable:next ruleNamedirectives in the code, so it does make some sense to include it in the same tool.I've wanted to add something like this for a while, but I'm not 100% convinced that it will be useful in practice. Much of the benefit of SwiftFormat's token-based parsing is that it can do sophisticated transformations without getting confused by things like arbitrarily-placed whitespace, comments, or recursive structures, which regex can't really cope with. It also can't replicate features like the existing
urlMacrorule's ability to conditionally insert animportif the rule is matched.The SwiftLint version also lets you restrict a rule to a particular token type, e.g. just identifiers, strings or comments, which I've not currently implemented - not because it's especially hard, but because I'm not sure how best to configure it on a per-regex basis given that we aren't using a hierarchical config format like YAML. I'm also not sure that would make a huge difference to the usefulness anyway.
Thoughts @calda? Is this something you'd find useful? Can you think of any improvements to the design?