-
Notifications
You must be signed in to change notification settings - Fork 221
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
Output a warning for unknown configuration rules in .swift-format
#612
Output a warning for unknown configuration rules in .swift-format
#612
Conversation
49c9a8d
to
cad7b65
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.
Can you update this now that #614 has been merged, which moved everything from the SwiftFormatConfiguration
module into SwiftFormat
?
6d3c19f
to
3a62daa
Compare
@allevato, I've addressed your first round of feedback:
Next up: potentially do source location. If you're happy with what is currently here, it would be okay by me to merge and follow up with an improvement. But, up to you! |
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.
@allevato ready for another review! I made the source locations work in a not entirely ugly way, I think!
@@ -12,8 +12,8 @@ | |||
|
|||
// This file is automatically generated with generate-pipeline. Do Not Edit! | |||
|
|||
enum RuleRegistry { | |||
static let rules: [String: Bool] = [ | |||
@_spi(Internal) public enum RuleRegistry { |
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.
@allevato, @_spi
is done here.
import SwiftSyntax | ||
import SwiftParser | ||
|
||
@_spi(Internal) import SwiftFormat |
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.
Reordered imports, but we should probably just setup formatter in #610. I'm happy to do it, just say the word ;)
// If an explicit configuration file path was given, try to load it and fail if it cannot be | ||
// loaded. (Do not try to fall back to a path inferred from the source file path.) | ||
// The actual config file that this frontend is going to use. | ||
let configURL: URL? |
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 need the config file URL later to figure out the source location, so this function got a little refactor in.
/// - parameters: | ||
/// - configuration: `Configuration` to verify | ||
/// - configURL: Configuration file `URL` to use in `Diagnostic.Location` | ||
private func verifyConfigurationRules(for configuration: Configuration, at configURL: URL) throws { |
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 meh on naming here. It does get the job done, but I don't think the name is 💯.
verify
implies that if the config is invalid, it will throw and the program will exit with an error.
Perhaps an explicit checkForUnrecognizedRules(in: config, at: path)
?
Also: I don't think it throws
anymore.
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.
checkForUnrecognizedRules(in: config, at: path)
sounds good to me. And yeah, remove the throws
if it's no longer necessary.
/// - parameters: | ||
/// - configuration: `Configuration` to verify | ||
/// - configURL: Configuration file `URL` to use in `Diagnostic.Location` | ||
private func verifyConfigurationRules(for configuration: Configuration, at configURL: URL) throws { |
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.
checkForUnrecognizedRules(in: config, at: path)
sounds good to me. And yeah, remove the throws
if it's no longer necessary.
|
||
/// Returns a `Diagnostic.Location` of the substring in an array of Strings. | ||
/// It's intented to find the location of a configuration Rule in an array of Strings representing the encoded JSON swift-format configuration file. | ||
private func diagnosticLocationFor(substring: String, in configuration: String, filePath: String) -> Diagnostic.Location? { |
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.
For what it's worth, I don't think this is really necessary. If someone mistypes a rule name, it won't take them long to figure out where in the configuration it is. And if we could get the information during decoding, it might be useful (still questionable though), but since this is just doing raw string searches, it's too prone to false positives if the invalid rule name matches something somewhere else in the configuration string.
Can you remove this and just emit a warning with no location?
Sounds great to me. I felt that it's too much ceremony, but liked the neat detail of having a line number. I'll do that, and some light cleaning up — that should be ready in a couple of hours. |
**Summary**: - Helps developers surface any errors and typos in their `.swift-format` config files by showing a warning when running `swift-format` if the configuration includes invalid rules. - The actual check is in `Frontend.swift`. - The dictionary of rules for verification is in `RuleRegistry`, which is made `public` to be able to import it in `swift-format` target. **Motivation**: - Closes swiftlang#607 Apply suggestions from code review Co-authored-by: Tony Allevato <tony.allevato@gmail.com> Cleaning up based on PR review
e982bac
to
5d45493
Compare
@allevato, alright, cleaned it up. Pained me to reduce clarity by removing source location, but you're right — re-reading config and doing raw string match is clumsy and buggy. |
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.
Looks good now, thank you!
…on/unknown-rules Output a warning for unknown configuration rules in `.swift-format`
Summary:
.swift-format
config files by showing a warning when runningswift-format
if the configuration includes invalid rules.Frontend.swift
.RuleRegistry
, which is madepublic
to be able to import it inswift-format
target.Example output:
Motivation:
Reviewing
I've madeRuleRegistry.rules
public. We could in theory use_@spi
instead.public
seems simple and okay — do you think that's fine?RuleRegistry
is now@_spi(Internal)
.swift-format
executable target is not tested. See Unit tests for swift-format CLI tool and for Plugins #611. I think I can start us off with a very simple test that tests specificallyFrontend.configuration()
.Potential improvements
This PR does not report the source location, hence theSource location reporting is in, but it's sliiiightly ugly.<unknown>
in the warning line. Hypothetically, we could read the configuration into an array of strings, and find the index of the string containing the unsupported rule, and construct the source location from that. Since the performance hit is negligible, that could be a nice touch.@allevato, how does that look to you? What else can I improve? If the general direction seems good, I would be happy to invest a bit more time and perhaps provide source locations.