Skip to content
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

Add syntax to swiftlint_version to allow for specifying minimum and maximum versions #5694

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alex-taffe
Copy link

@alex-taffe alex-taffe commented Jul 24, 2024

Currently, the configuration supports a swiftlint_version option. This option does not work for our workflow because we have many branches that may not be up to date with main/master. However, there are breaking changes that occur in SwiftLint, such as redundant_optional_initialization changing between 0.53.0 and 0.55.1, causing our team to not have consistency in their linting.

This adds new syntax to swiftlint_version that will compare to the current version installed and throw a warning/error if the installed version does not match the pattern the user specified

warning: Currently running SwiftLint 0.55.1 but configuration specified minimum version 0.55.3.

The syntax is as follows:

swiftlint_version: 0.54.0
swiftlint_version: ">0.54.0"
swiftlint_version: ">=0.54.0"
swiftlint_version: "<0.54.0"
swiftlint_version: "<=0.54.0"

@SwiftLintBot
Copy link

SwiftLintBot commented Jul 24, 2024

1 Error
🚫 Could not build branch
1 Warning
⚠️ This PR may need tests.

Generated by 🚫 Danger

@alex-taffe
Copy link
Author

@SimplyDanny @mildm8nnered if you could take a look at this when you get a chance, thanks!

@SimplyDanny
Copy link
Collaborator

I see where you are coming from and why this would help, yet I was wondering whether a new separate key is really the way to go. The list is continuously growing and there is the problem of conflicting options. What about minimum_swiftlint_version: 0.54.0 and swiftlint_version: 0.53.0 for example? So there should probably be some consistency checks.

What I was thinking about is an extension of swiftlint_version in a way that it supports both fixed versions and a range of version given a minimum version. Something like the syntax swiftlint_version: >=0.54.0 could be conceivable. What's your opinion on that?

@alex-taffe
Copy link
Author

alex-taffe commented Jul 31, 2024

Yeah I think I actually like that better, what're your thoughts on syntax? I kinda like Dart's. Traditional and caret. Then if we can't parse, just throw a fatal error.

image image

Plus '>=1.2.3 <2.0.0' for inclusive

@alex-taffe
Copy link
Author

Also what's the minimum version of macOS that SwiftLint supports? Would love to use new SwiftRegex but it's macOS 13 or newer only

@SimplyDanny
Copy link
Collaborator

Also what's the minimum version of macOS that SwiftLint supports? Would love to use new SwiftRegex but it's macOS 13 or newer only

SwiftLint still supports macOS 12. We'd need good reasons to change that. While I fancy as well with SwiftRegex, it's not enough reason to change the minimum deployment version. But that might change when macOS 15 is out.

About the syntax: I personally don't like the caret style so much. Only the three variants

  • 0.54.0
  • >0.54.0
  • >=0.54.0

seem to be enough. There is no good reason for <0.54.0 or <=0.54.0, is there?

@alex-taffe
Copy link
Author

In practice I’m guessing the less than or inclusive versions wouldn’t actually be used much (I doubt we ever will), but doesn’t seem like that much more work so might as well just do it while I’m in there. I’m fine with omitting caret syntax

@SimplyDanny
Copy link
Collaborator

In your implementation it could be helpful to know that Version objects are Comparable. 😉

@alex-taffe alex-taffe changed the title Add minimum_swiftlint_version as a configuration option Add syntax to swiftlint_version to allow for specifying minimum and maximum versions Aug 1, 2024
@alex-taffe alex-taffe force-pushed the minimum-version branch 2 times, most recently from 88111b9 to 28e4bd4 Compare August 1, 2024 14:49
@alex-taffe
Copy link
Author

@SimplyDanny updated

Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good starting point! Please also add some tests for all the cases.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
// make sure the remaining string is just a version string of numeral.numeral.numeral
versionString = versionString.trimmingCharacters(in: .whitespacesAndNewlines)
guard versionString.range(of: #"^\d+\.\d+\.\d+$"#, options: .regularExpression) != nil else {
showInvalidVersionStringError()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be more specific for the error case of an invalid version number format.

@alex-taffe
Copy link
Author

@SimplyDanny how do you suggest I approach tests on this? AFAIK, XCTest can't handle pulling out info from stdout nor can it intercept calls to abort(). I could refactor the method so it throws out an error enum with an intermediary that actually calls abort, but seems like a lot of boilerplate. Does bazel have some other better way to tackle this?

@alex-taffe
Copy link
Author

@SimplyDanny just seeing if you had any thoughts on this. Apologies for the follow up, this is just rapidly causing chaos amongst our 9000 internal branches (which is a whole separate issue but)

Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
)
}

func compareVersionString(_ versionString: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if "version constraint" should be a struct that implements the comparison logic. At least this struct can be tested then.

Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved

// make sure the remaining string is just a version string of numeral.numeral.numeral
versionString = versionString.trimmingCharacters(in: .whitespacesAndNewlines)
guard versionString.range(of: #"^\d+\.\d+\.\d+$"#, options: .regularExpression) != nil else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the Version initializer should check this.

Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more style issues ...

Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
Source/SwiftLintCore/Models/Configuration.swift Outdated Show resolved Hide resolved
@alex-taffe
Copy link
Author

@SimplyDanny yeah sorry I pushed a bunch of stuff earlier then got distracted with some fires at work, I was working on refactoring into a struct anyways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants