Add support for environment variables in Swiftlint configs #99
Conversation
|
@ashfurrow it would be great if we could do a release as well with these changes so we can use this release internally at WeTransfer :) |
There was a problem hiding this comment.
Looks great! Nice test coverage. Just a few pieces of feedback. I'd be happy to release after this is merged, or I can add you as an owner to the gem. Just send me your email address (you'll need an account on https://rubygems.org just like CocoaPods Trunk).
lib/danger_plugin.rb
Outdated
|
|
||
| # Find all requested environment variables in the given string and replace them with the correct values. | ||
| def parse_environment_variables(file_contents) | ||
| file_contents.gsub(/\$\{([^{}]+)\}/) do |environment_variable| |
There was a problem hiding this comment.
For this kind of regexing, it'd be nice to have a few comments describing what this does. Maybe a link out to the docs to read more.
And environment_variable is a very... Swift variable name 😅 I'd go with something shorter, like env_var.
There was a problem hiding this comment.
haha, I'm a bit swifty! Thanks for that feedback. I'll add a describing documentation, that makes sense!
| @@ -316,6 +318,27 @@ module Danger | |||
| expect(status[:warnings]).to_not be_empty | |||
| expect(status[:errors]).to be_empty | |||
| end | |||
|
|
|||
| it 'parses environment variables set within the swiftlint config' do | |||
| ENV['ENVIRONMENT_EXAMPLE'] = 'excluded_dir' | |||
There was a problem hiding this comment.
I took a look to see if there were a better way to stub out this environment variable, but apparently there isn't! However, I would move ENV['ENVIRONMENT_EXAMPLE'] = nil into an after(:each) do ... end block, for two reasons:
- further tests with this variable shouldn't have to remember to reset it, and
- if this test raises and error halfway through, we want Rspec to reset the variable for us so the environment is clean for the next test.
|
@ashfurrow it should all be good now. I've sent you a private message on twitter with my email. |
There was a problem hiding this comment.
Looks good! Merge when ready.
I've added you as an owner. Releasing is pretty straightforward, but maybe after you go through it you could send a PR documenting these steps (and any others you find helpful)?
- Checkout master. Pull.
- Edit
/lib/version.rb's version number. - Edit the Changelog (add
- Nothing yet!under "Current Master" and move existing contents to a new header with the new version number). gem build [gemspec name].gem push [.gem file](these are like.ipas).- Commit. Push.
- Tag with the version number.
git push --tags.
That should do it. And thanks again!
Swiftlint configs support environment variables since 0.21.0 (See realm/SwiftLint#1512).
As we were filtering out excluded and included paths if they didn't exist on disk, environment variables would not reach Swiftlint and would not get parsed. With this PR we're implementing support for environment variables just like Swiftlint.
An example usage can be found here, where we use it with WeTransfer to reuse config files across repositories: https://github.com/WeTransfer/WeTransfer-iOS-CI/blob/master/SwiftLint/.swiftlint-source.yml#L50