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 Ruby and Swift linters and PR template #27

Closed
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dcordero
Copy link
Collaborator

dcordero commented Mar 31, 2018

  • Add Swift and Ruby linters
  • Define PR template for GitHub
  • Define lint lane
  • Add lint step to circleci
  • Fix linter issues in Ruby and Swift code

@dcordero dcordero changed the title Add linters and PR templates Add linters and PR template Mar 31, 2018

@dcordero dcordero changed the title Add linters and PR template Add Ruby and Swift linters and PR template Apr 1, 2018

@carolanitz
Copy link
Member

carolanitz left a comment

I like this a lot!
A couple of comments inline. What I would definitely like is to adjust Code style.rtf in Docs. I think a good example is here: https://github.com/ResearchKit/ResearchKit/blob/master/docs-standalone/coding-style-guide.md
I might make sense to separate the PR into one that adds the linter and required files and one that adjusts all the files according to the new rules. What do you think about that ?

<!-- Thanks for contributing to _vlc-ios_! Before you submit your pull request, please make sure to check the following boxes by putting an x in the [ ] (don't: [x ], [ x], do: [x]) -->

### Checklist
- [ ] I've run `bundle exec fastlane test` from the root directory to see all new and existing tests pass

This comment has been minimized.

@carolanitz

carolanitz Apr 3, 2018

Member

How do you feel about making this part of circle ci and adjust the text?

This comment has been minimized.

@dcordero

dcordero Apr 9, 2018

Collaborator

Good idea, I added the step test to the ci lane. Due to the fact that tests are broken at the moment in master, this change will make the tests also fail from now on in this PR, till the PR fixing the tests is merged.


### Checklist
- [ ] I've run `bundle exec fastlane test` from the root directory to see all new and existing tests pass
- [ ] I've followed the vlc-ios code style and run `bundle exec fastlane lint` to ensure the code style is valid

This comment has been minimized.

@carolanitz

carolanitz Apr 3, 2018

Member

can we adjust the code style.rtf (which probably should turn into an .md file) and link to it here ? :)

This comment has been minimized.

@dcordero

dcordero Apr 9, 2018

Collaborator

I could not agree more... done

@@ -20,6 +20,7 @@ jobs:
- .bundle
- restore_cache:
key: v1-pods-{{ checksum "Podfile.lock" }}
- run: brew install swiftlint

This comment has been minimized.

@carolanitz

carolanitz Apr 3, 2018

Member

I think this might also need to go into the README for requirements :)

This comment has been minimized.

@dcordero

dcordero Apr 9, 2018

Collaborator

Good point, I have opted in by using swiftlint via Cocoapods so it does not introduce additional steps.

temp_changelog = changelog.split("tvOS")
end
temp_changelog = changelog.split(platform)
temp_changelog = changelog.split('tvOS') if temp_changelog.count <= 1

This comment has been minimized.

@carolanitz

carolanitz Apr 3, 2018

Member

nice! I didn't know that's how you can write this

This comment has been minimized.

@dcordero

dcordero Apr 9, 2018

Collaborator

Me neither tbh, rubocop made this change 😆

@carolanitz
Copy link
Member

carolanitz left a comment

I just had two minor things the rest looks really good!! Thank you so much ❤️

@@ -41,6 +41,7 @@ end

lane :ci do
lint
test

This comment has been minimized.

@carolanitz

carolanitz Apr 10, 2018

Member

I would move the running of tests step after the building step :)

This comment has been minimized.

@dcordero

dcordero Apr 10, 2018

Collaborator

Ok, I do not have a strong opinion about that. Changed

In fact as soon as we have tests for both targets, the test step will make redundant the building steps, because it will build the App to run the apps.

But that is a next step...

```obj-c
BOOL isFirstTimeReading;
int age;
Array *myArray;

This comment has been minimized.

@carolanitz

carolanitz Apr 10, 2018

Member

NSArray ? :D

This comment has been minimized.

@dcordero

dcordero Apr 10, 2018

Collaborator

Good catch

@carolanitz

This comment has been minimized.

Copy link
Member

carolanitz commented Apr 10, 2018

squashed and merged with fad3d68

@carolanitz carolanitz closed this Apr 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment