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.

Sign up for GitHub

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

Jump to bottom

Add Swiftlint #trivial #681

Closed
wants to merge 8 commits into from
Closed

Add Swiftlint #trivial #681

wants to merge 8 commits into from

Conversation

Iron-Ham
Copy link
Contributor

@Iron-Ham Iron-Ham commented Apr 20, 2017 •
edited

Replaces #642

Issue ref: #394

Integrates SwiftLint into DangerBot.
You may want to append a ?w=1 to the end of the files-changed url to exclude whitespace-only changes.

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have reviewed the contributing guide

#trivial

@iglistkit-bot
Copy link

iglistkit-bot commented Apr 20, 2017 •
edited

1 Warning
⚠️ Big PR

SwiftLint found issues

Warnings

File Line Reason
MixedDataViewControllerUITests.swift 86 Line should be 140 characters or less: currently 201 characters
MonthSectionController.swift 18 Line should be 140 characters or less: currently 167 characters
MixedDataViewController.swift 76 Force casts should be avoided.
MixedDataViewController.swift 79 Force casts should be avoided.
MixedDataViewController.swift 26 Line should be 140 characters or less: currently 142 characters
NestedAdapterViewController.swift 51 Force casts should be avoided.
SearchViewController.swift 25 Line should be 140 characters or less: currently 702 characters
SelfSizingCellsViewController.swift 40 Line should be 140 characters or less: currently 260 characters
SelfSizingCellsViewController.swift 41 Line should be 140 characters or less: currently 264 characters
SelfSizingCellsViewController.swift 42 Line should be 140 characters or less: currently 393 characters
FullWidthSelfSizingCell.swift 77 Line should be 140 characters or less: currently 142 characters
ManuallySelfSizingCell.swift 77 Line should be 140 characters or less: currently 142 characters
MessagesViewController.swift 26 Line should be 140 characters or less: currently 177 characters
TodayViewController.swift 27 Line should be 140 characters or less: currently 177 characters
NestedAdapterViewController.swift 52 Force casts should be avoided.

Generated by 🚫 Danger

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@Iron-Ham Iron-Ham changed the title Swiftlint #trivial Swiftlint Apr 21, 2017
@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@jessesquires jessesquires mentioned this pull request Apr 21, 2017
Closed
@jessesquires
Copy link
Contributor

@rnystrom -- still having trouble getting this imported. any updates?

@rnystrom rnystrom changed the title #trivial Swiftlint Add Swiftlint Apr 24, 2017
@rnystrom
Copy link
Contributor

Ok so this is adding about 35mb to the repo from including the SwiftLint framework 3x from each of the examples. Wow. Our internal tools flagged this and wont let us import it b/c of the size.

I'm not totally sure what the best course of action here should be. Obviously the main issue is having 3x separate example projects, each with their own Podfile, and the fact that we check in all Pod data.

Couple options:

  • We drop all the SwiftLint stuff and just keep the lint changes to .swift files
  • We update all of the examples to be a single project/workspace so we only SwiftLint import once
  • We drop this altogether ☹️

@Iron-Ham
Copy link
Contributor Author

@rnystrom surprised it's 35mb.
I think short-term, the right move may be to take the changes to the .swift files and drop SwiftLint from the example projects -- it won't flag errors during development of the Example project, but the bot should still flag errors on the PR.

Restructuring into one xcworkspace sounds like the right longer-term move to me.

@Sherlouk
Copy link
Contributor

Sherlouk commented Apr 24, 2017 •
edited

Another option is to get the framework (1 version of it) store that in a directory (e.g. Examples/Swiftlint) and then reference that with a build script from all 3 projects. (So stop using CocoaPods to get Swiftlint).

I'd honestly prefer this!
@rnystrom @heshamsalman

edit//
It's also not helping that there is a file.zip in all three examples "file.zip" of 3.5mb each which from what I can tell is not being used. (saves 10mb) and just having 3 instances of SL is not helping - which if we went with my idea would save us a bunch more!

@jessesquires
Copy link
Contributor

I think @Sherlouk 's idea is good.

That's how the local IGListKit pod is working -- all examples just reference ../../IGListKit

@Iron-Ham
Copy link
Contributor Author

Iron-Ham commented Apr 24, 2017 •
edited

Occam's Razor 😎
Seems like the right move to me. I'll make the change soon

@jessesquires
Copy link
Contributor

cc @rnystrom ?

@rnystrom
Copy link
Contributor

On board w/ @Sherlouk's plan, sounds great!

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@Iron-Ham Iron-Ham changed the title Add Swiftlint Add Swiftlint #trivial May 10, 2017
@Iron-Ham
Copy link
Contributor Author

@Sherlouk @jessesquires @rnystrom

Should be a good deal smaller now. 👍

@Sherlouk
Copy link
Contributor

Changes look a lot nicer, will leave it to the insta crew to check on Phabricator! Hopefully we can get it in 🙏

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@heshamsalman updated the pull request - view changes

@Sherlouk
Copy link
Contributor

@rnystrom @jessesquires Think we could try and see if Phabricator will except this some time this week? Would be nice to get this in (and I get that it's a pain to keep rebasing 😅)

@jessesquires
Copy link
Contributor

@Sherlouk - yep, everything looks alright to me!

@facebook-github-bot
Copy link
Contributor

@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Iron-Ham
Copy link
Contributor Author

Ayyy 👍

@jessesquires
Copy link
Contributor

jessesquires commented May 15, 2017 •
edited

Ok, bad news:

So import succeeded but land failed internally because the binary files are too large. 😞

The best path forward is probably to run this on CI, but not include SwiftLint in the repo? Not 100% on what the best option is.

@rnystrom is out for a bit, when he returns we can sort this out. 👍

Sorry for the churn @heshamsalman ! 😅

@Iron-Ham
Copy link
Contributor Author

Iron-Ham commented May 15, 2017 •
edited

@jessesquires I could alternatively change the SwiftLint script to rely on a user's HomeBrew installed copy of SwiftLint (instead of a locally referenced binary), and echo installation instructions if they don't have it installed?

And no worries re: churn

@jessesquires
Copy link
Contributor

jessesquires commented May 16, 2017 •
edited

@heshamsalman -- that sounds awesome! Let's definitely do that.

That's how we handle jazzy. User must have it installed. (Though we don't provide a helpful message about how to install. that would be nice)

@jessesquires
Copy link
Contributor

@heshamsalman -- because of how the importing system works, we'll need to open a new PR 😄

@Iron-Ham
Copy link
Contributor Author

@jessesquires haha classic. I'll re-open soon

@Iron-Ham Iron-Ham closed this May 16, 2017
This was referenced May 16, 2017
Closed
Closed
@jessesquires
Copy link
Contributor

thanks so much @heshamsalman ! 🎉

@BasThomas
Copy link
Contributor

User must have it installed. (Though we don't provide a helpful message about how to install. that would be nice)

Might be a good idea to create an issue for that? :)

@jessesquires
Copy link
Contributor

@BasThomas Yep! 😊

facebook-github-bot pushed a commit that referenced this pull request May 16, 2017
@facebook-github-bot
Implemented SwiftLint, new PR
02f28fc 
Summary:
Replaces #642, #681

Issue ref: #394

Changes from last PR:
- Deleted the shared SwiftLint folder, incl. all files (On the plus side, did this in the quest to get this PR in realm/SwiftLint#1513)
- Changed the build script such that it runs the user's `HomeBrew` installation of SwiftLint instead of the local copy

Integrates SwiftLint into DangerBot.
You may want to append a ?w=1 to the end of the files-changed url to exclude whitespace-only changes.

- [ ] All tests pass. Demo project builds and runs.
- [ ] I added tests, an experiment, or detailed why my change isn't tested.
- [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
- [ ] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md)
Closes #741

Differential Revision: D5068134

Pulled By: jessesquires

fbshipit-source-id: 68d6a57e0072672e38eeb94908d00f26bbd68fbc
@BasThomas BasThomas mentioned this pull request May 16, 2017
Closed
@BasThomas
Copy link
Contributor

Created #744. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Iron-Ham @iglistkit-bot @facebook-github-bot @jessesquires @rnystrom @Sherlouk @BasThomas