[Draft] Add buildFilesAreOrderedRule #4
Conversation
|
First, I absolutely love this rule! It's excellent. I had a feeling the way I did that extension XCLinter {
public static let defaultRules: [Rule] = [
embeddedBuildSettingsRule,
{ MyNewRule().doStuff(with: $0) }
]
}
final class MyNewRule {
// state! things! whatever!
private var value: Int
init() {
self.value = 5
}
func doStuff(with env: XCLinter.Environment) -> [Violation] {
return []
}
}I was worried that removing the text would be an issue, but I figured since the URL is available, if a I'm also kinda surprised that the contents of the |
|
Another point you bring up is the mapping of rule identifier to actual code that puts the corresponding |
|
I just began poking around a little, because I was curious. I'm now more confident (but not 100%) that the information you need is available via the Here's some relevant stuff: I think this proves that, at least in this specific case, the order and name information is available. Not in the most convenient form, mind you, but at least it looks like you may not need to mess around with the text anymore! |
Oh interesting. I remember looking in the past and finding that the items were unordered. But I might be thinking of a different API. The snippets above look like the items in a PBXGroup, which is ordered (since they are in an array). The order of items in a group is preserved by Xcode, and can be manually changed by the user. The issue I'm trying to address is the items in the PBXBuildFile and PBXFileReference sections of the main object collection. This is technically an unordered collection. The identifier is the key and the object is the value. But, Xcode writes these objects out to disk in order. So at the top of the project file There's this: The goal of this rule is to avoid churn in these sections when bad merges result in items being out of order. Xcode will tolerate these items being out of order, since it's technically an unordered collection. But my enforcing that items must always be ordered, you can avoid this churn. I'll take another look at |
|
Ok yeah, it looks like the And it looks like |
A big ol' switch is totally fine. In my own project I started that way. I just found it convenient to have the name of the rule defined with the logic. But even in this approach I had a big array of all the rule types. So I'm not sure if there's any real "magic" approach that would be worth it. Certainly nothing using But there's another point to consider maybe in favor of defining types for rules. As rules get more complex, some of them may allow for customization. Imagine a rule that enforces items in groups being in alphabetic order. Such a rule might have a parameter to specifies how subgroups are handled. Either ordered with the files, ordered separately at the top of the group, or at the bottom. Would it be easier to customize such a rule (with data from a config file) if it had a well defined type? |
|
Updated with a commit to construct a type to contain the logic, as you suggested. And it's pretty nice! |
|
Ah I totally didn't understand what this rule did, but I think I do now. Sorry about the wild goose chase!
I don't know! I've typically found this kind of general-purpose parameters tough to handle with Codable. But, I think it's doable. I'm absolutely not opposed to a type. I was just playing around. Either way, I think what you have here is really useful. And I also really appreciate you putting this time in. Do you think there's more you want to do? It feels pretty ready to me. |
|
@mattmassicotte Feels ready to merge to me! Go for it! |
This adds the
buildFilesAreOrderedRuleand some simple tests to validate it.The
buildFilesAreOrderedRuleverifies that the items in the PBXBuildFile and PBXFileReference sections of the project file are ordered by their identifiers. In normal use, Xcode tolerates items that are out of order, but writes them out in order. This can create churn on a large project files with several contributors. And this frequently occurs on bad merges or rebases, where the author is not careful to maintain the order when resolving conflicts.buildFilesAreOrderedRulefollows the existing pattern of being a pure function, but this ends up being a bit cumbersome. I added several utility functions to make this code easier to understand. And while these are private, it feels odd having them at the global level. I tried making them nested functions ofbuildFilesAreOrderedRule(), but that was even harder to read. If these methods were members of specific type, then there would be no risk of collisions with other functions elsewhere in the library. This rule also uses anNSRegularExpressionto parse lines in the project file, and this also has to be a global value.This rule does not operate on the parsed
XcodeProjmodel, instead it operates on the raw text of the project file. Luckily,Environmentmaintains the URL of the xcodeproj bundle on disk, so it's easy to read this data in. But if there are other rules in the future that also need to operate on the text, then it may be wasteful to read it in multiple times.I copied the tests in
EmbeddedBuildSettingsRuleTestsand updated them for this new rule, using a test project file I added. This project includes out of order entries in both the PBXBuildFile and PBXFileReference sections.You should feel empowered to make any and all edits you deem appropriate, or even reject this PR outright. I'm excited to contribute to this project, but I acknowledge not much is setup yet and you may have a vision for where you want it to go. Thank you!