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

Notify a new release on running danger-swift #505

Merged
merged 6 commits into from
Mar 3, 2022
Merged

Notify a new release on running danger-swift #505

merged 6 commits into from
Mar 3, 2022

Conversation

417-72KI
Copy link
Member

@417-72KI 417-72KI commented Feb 4, 2022

Some tools (e.g. carthage, gh, etc...) notify when new version is released.
I added a version checker to fetch a latest release in GitHub, and notify if running version is older than latest.

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR :) Great idea!
Just few minor comments:

}
}

static func fetchLatestVersion() -> String? {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be private

Copy link
Member Author

@417-72KI 417-72KI Feb 7, 2022

Choose a reason for hiding this comment

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

fixed!

}

extension VersionChecker {
static let logger = Logger()
Copy link
Member

Choose a reason for hiding this comment

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

This Logger won't have the right set up, which is made based on the parameters passed to Danger Swift, not a big deal, but would be nice to get it from the runner if possible (is created at line 25)

Copy link
Member Author

Choose a reason for hiding this comment

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

#505 (comment) may also resove it 😁

Sources/RunnerLib/VersionChecker.swift Outdated Show resolved Hide resolved
import Foundation

public enum VersionChecker {
public static func checkForUpdate(current currentVersionString: String) {
Copy link
Member

Choose a reason for hiding this comment

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

Can inject ShellExecutor and Logger (which anyway can be needed), so that we can also have some tests for this logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed VersionChecker enum to class, which make it injectable 😄.

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

If you merge master CI should pass.

import Version
import Foundation

public class VersionChecker {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick:

Can this be a struct? I prefer having structs if possible, following the Apple's suggested approach

Consider the following recommendations to help choose which option makes sense when adding a new data type to your app.
Use structures by default.
Use classes when you need Objective-C interoperability.
Use classes when you need to control the identity of the data you're modeling.
Use structures along with protocols to adopt behavior by sharing implementations.

from https://developer.apple.com/documentation/swift/choosing_between_structures_and_classes

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!

Comment on lines 10 to 13
public init(
shellExecutor: ShellExecutor = .init(),
logger: Logger
) {
Copy link
Member

Choose a reason for hiding this comment

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

Given this is now injectable, can we also add some tests for this please?

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

@417-72KI
Copy link
Member Author

417-72KI commented Feb 19, 2022

@f-meloni

If you merge master CI should pass.

There is a problem to build SwiftLint on Linux (5.5.2), which makes CI fail 🙄

Latest version of SwiftLint may be resolve it and I made another PR.
#509

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

🚀

@417-72KI 417-72KI merged commit ec804f8 into danger:master Mar 3, 2022
@417-72KI 417-72KI deleted the version-check branch March 3, 2022 06:07
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.

None yet

2 participants