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

Managed swiftlint integration #28

Merged
merged 11 commits into from
Feb 16, 2017
Merged

Managed swiftlint integration #28

merged 11 commits into from
Feb 16, 2017

Conversation

thiagofelix
Copy link
Collaborator

@thiagofelix thiagofelix commented Feb 14, 2017

PR inspired by issues #23 and #25

As of now danger-swiftlint doesn't know which version of swiftlint is installed on the host computer, it either expect it to be already installed or it installs at runtime using brew install swiftlint. The problem of this is that we can't predict which version of swiftlint is installed and therefore we don't know if it will work or not with the arguments and configuration we are running with. This situation leads to a potencial flaky behaviour increasing the maintenance burden.

What I want to explore in this PR is a different way to install swiftlint on the host computer and ensure that it will be always using a predicted version. Instead of relying on a global installed swiftlint application I wanted to bundle the swiftlint application together with danger-swiftlint as a gem extension. This way everytime the user install danger-swiftlint it will also be installing a particular version of swiftlint that comes with it.

As a consequence of pairing danger-swiftlint with swiftlint I think it makes sense for them to use the same version number. Meaning if you install danger-swift@0.16.1 you will be using swiftlint@0.16.1 everytime you run your dangerfile.

@ashfurrow I would be happy to hear your feedback if you think it makes sense.

Following is the list of tasks that I am expecting to tackle:

  • Sync danger-swiftlint and swiftlint versions
  • Update the gem to build swiftlint native extension
  • Update the plugin to use the extension interface to run swiftlint
  • Ensure tests are still passing

@thiagofelix
Copy link
Collaborator Author

thiagofelix commented Feb 15, 2017

With these changes when the user runs gem install danger-swiftlint during the build process gem will download the portable_swiftlint.zip file present on the release and store that close to the gem to be used at runtime.

Here is how the command output would look like:

gem install ./danger-swiftlint-0.16.1.gem
Building native extensions. 
Successfully installed danger-swiftlint-0.16.1
Parsing documentation for danger-swiftlint-0.16.1
Done installing documentation for danger-swiftlint after 0 seconds
1 gem installed

The line Building native extensions. This could take a while... is when it is downloading the portable zip file from the github releases page. After that danger-swiflint will have its own version of swiflint to use

Copy link
Owner

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Great, looks good! One question above. Really dig the test coverage and documenting comments. Could you add a Changelog entry as well?

@@ -1,3 +1,4 @@
module DangerSwiftlint
VERSION = "0.3.0".freeze
VERSION = "0.16.1".freeze
Copy link
Owner

Choose a reason for hiding this comment

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

Is this meant to mirror the SwiftLint version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is, my goal to mirror both versions I believe is two fold 1) It is easy/quick for us to know which version of swiftlint will be used and what it is capable of and how much we need to "workaround" any limitation it has 2) From the user perspective they will be able to choose which version makes sense for them, perhaps they are not ready for a upgrade or they want to ensure always will use a given swiftlint version, etc, leading to a more predictable less flaky build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And of course, versions prior to 0.16.1 wouldn't benefit from this behavior and if you have gem "0.3.0" on your Gemfile that will still work as it is today and will download the latest swiftlint if it is not installed yet.

@thiagofelix
Copy link
Collaborator Author

Updated changelog @ashfurrow =)

@ashfurrow ashfurrow merged commit 050f3bc into ashfurrow:master Feb 16, 2017
@thiagofelix
Copy link
Collaborator Author

Thanks for merging, looking forward to see how the new model performs into the wild. I'll extend the README to explain better the relationship of this library with swiftlint (versioning, isolated install, etc).

@ashfurrow
Copy link
Owner

Cool cool, thanks for contributing!

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.

2 participants