Fix issue that prevented the inclusion of a configuration file from a parent folder #3491
Conversation
|
The issue has been that, when only one single configuration file was provided, the relative path management for the included & excluded paths, as implemented here, was never called. That only happens for more than one configuration... To fix this, I added relative path handling for the first configuration file, so that even without more configuration files, the included / excluded paths are put in proper relation to the root directory. |
|
Seeing very weird SwiftLintBot reports here... All new violations reported by the SwiftLintBot are from files that are excluded in Sourcery's So to me it seems like the new violations are in fact intended behavior! But then, why are they new and why does this PR change the behavior? As a reference: This test PR that basically does nothing except for renaming a variable doesn't cause new warnings. Primarily, I'd like to understand what the intended behavior is (meaning whether the new violations should not be there or should have always been there). Any ideas, @jpsim, @marcelofabri, @norio-nomura? |
Yeah it's not working correctly, see #3533 |
jpsim
force-pushed
the
feature/3485-fix-relative-paths
branch
from
6691119
to
129bd52
Compare
|
PR looks great! I hope you don't mind @fredpi I rebased and force-pushed to this branch and will merge when CI passes. |
|
This is actually a breaking change, and in fact broke a SwiftLint integration with a closed source project of mine. I believe this covers the scenario where the breakage is happening: The custom config looks like this: included:
- Dir1/Dir2/Sources/MyFile.swift
analyzer_rules:
- unused_importAnd I'm running SwiftLint from the Before this PR, it worked well: With this PR, it's failing to find files to lint: So unfortunately I'll be reverting this to avoid this breaking change until we can think through a possible solution. |
|
Reverted in b66f7c7 |
|
@jpsim I'm sorry for breaking things again – it seems like there were good reasons that nobody really touched the whole configuration system for some time... 🥲 If I understand your example correctly, we have different ideas of where the I documented that behavior in the
This PR only fixed an inconsistency with the design I chose, so if we decide that the However, I still think that it is better if the paths are relative to the location of the config file:
Of course, this change of concept is a breaking change and I should have better explained it in #3058 for us to be able to discuss its consequences before releasing it – also without the bug / inconsistency this PR fixed, we would have noticed the breaking character of this change earlier... But now, that it is released, I think the best way to move forward is to leave it with the current state, remerge this PR and maybe drop a breaking notice in the next release notes (although parts of the breaking change already happened in 0.41.0). But maybe it's just that I misunderstood the example you provided, so please tell me, if that's the case! |
This is why I really appreciate your work in this area, especially the added tests, documentation and perhaps most importantly consideration about how this should all work, which I'll admit I still find pretty difficult to reason about. After reading through your last comment and thinking it through, I completely agree that we should remerge this PR and that the behavior is what makes most sense. However, I think there's still value in allowing out-of-tree configuration files. In my case, I don't want to dirty the git state in the repo being linted and the configuration files that I'm generating in I'd still like to find a way to support that. Do you agree we should find a way to support this? Do you think it would make sense for configuration files that are loaded from locations that aren't descendant of the current working directory to have their in |
Actually I found a simple solution, at least for my use cases. Out-of-tree configuration files can use absolute paths, which works well today. included:
# Before
- Dir1/Dir2/Sources/MyFile.swift
# After
- /src/MyProject/Dir1/Dir2/Sources/MyFile.swift
analyzer_rules:
- unused_import |
|
Re-merging in #3542 |
Yes, me too! There are so many use cases that need to be considered and when adding a fix for one use case, it's hard not to break another use case 😅
Yes, definitely!
I think such an approach would be rather irritating, e. g.: What behavior to expect for the use case where a configuration file from one directory above is used? Actually the issue this PR addressed (#3485) is an example for that: Their command is The absolute path approachBut I think the solution you discovered for your use case (specifying an absolute path starting with Now, without having it tested yet, I'd claim that this does not only work for your use case (some distant path), but also when specifying an absolute path to a file that lives in a directory below the configuration file. Internally, the absolute path may get transformed to a relative path, but that wouldn't break the functionality. So I suggest the following:
|
Closes #3485.