Skip to content
This repository was archived by the owner on Sep 20, 2023. It is now read-only.

Enable SwiftLint and fix issues #2292

Merged
merged 4 commits into from
Oct 17, 2018
Merged

Enable SwiftLint and fix issues #2292

merged 4 commits into from
Oct 17, 2018

Conversation

rnystrom
Copy link
Member

I'm preparing to do some more automation with the repo to speed up PR review and unify code formatting. First step is to enable SwiftLint to take care of basic issues.

@Huddie
Copy link
Collaborator

Huddie commented Oct 15, 2018

@rnystrom ever considered SwiftFormat ?

Sent with GitHawk

@rnystrom
Copy link
Member Author

Ya but seems like this is more mature, and using both would be pretty redundant right?

Sent with GitHawk

@Huddie
Copy link
Collaborator

Huddie commented Oct 15, 2018

Ya one warns/produces errors and one just formats it all for you. So having both would essentially but just having SwiftFormat (unless you configured them differently)

You can place swiftFormat in bitrise so that it formats it according to a given standard whenever a PR is created but not on the local machines. So it gives devs to program however they desire but the product that gives merged will always be according to GitHawk style guide

Sent with GitHawk

@rnystrom
Copy link
Member Author

@Huddie any pointers on how how to make that all work? I actually don’t mind making people address the linter vs me having to keep merging PRs for formatting 😛

Really just want to speed up PR turn around and spend less time nitpicking.

Sent with GitHawk

@Huddie
Copy link
Collaborator

Huddie commented Oct 15, 2018

@rnystrom sure. I’ll try getting it up and running.

Sent with GitHawk

@j-f1
Copy link

j-f1 commented Oct 15, 2018

My experience with Prettier, a formatter for web languages, is that it makes it a lot easier to write code since you can type it all on one line and hit save so it looks nice without having to manually indent the code.

@Huddie
Copy link
Collaborator

Huddie commented Oct 15, 2018

@j-f1 You type everything on 1 line? Do you mind giving some more background? Are you suggesting prettier for GitHawk or just your experience with a formatter such as SwiftFormat?

I like the idea of SwiftFormat in Bitrise more than running each time you build simply because people program with different spacing etc and no need to force a style on you while programming. SwiftFormat in bitrise should format the code once you request a PR so that when @rnystrom reviews it, it’s perfectly formatted according to GitHawk standards.

I’ll admit, one downside of having it run on Bitrise is that each PR needs to be tested. Not sure how many tests we get to run since I’ve seen PRs without tests

Sent with GitHawk

@j-f1
Copy link

j-f1 commented Oct 15, 2018

@Huddie I’m just providing my experience with formatters in general. Usually the difficulty I have with code styles is remembering to follow standards that are different from my own, but the differences aren’t as noticeable when reading the code. I’d suggest not auto-fixing issues in CI since that would add the noise of another commit to each PR.

@Huddie
Copy link
Collaborator

Huddie commented Oct 15, 2018

@j-f1 🤔 From my experience:
Having it format on builds doesn’t allow you to program how you typically do (cause every build reformats) + its slows down the compile time
And having swift lint is not an issue if there aren’t many issues but:

  • Not allowing builds because of formatting is a PAIN
  • Warnings work but don’t enforce anything. people will ignore them. So you’ll have to report them in the CI or something which is fine I suppose.
    I don’t really see the downside of having the CI commit a formatting change. @j-f1 Thoughts?

Sent with GitHawk

@j-f1
Copy link

j-f1 commented Oct 15, 2018

The editor integrations I’ve used typically reformat the file when you save. This ensures you follow the style guide while not throwing errors in your face when you don’t. Also, it doesn’t slow builds down because you only format the one file you just saved and that’ll probably happen faster than you can start the build.
I agree that lint issues shouldn’t stop you from building — as long as the code is syntactically correct, it should run.

@Huddie
Copy link
Collaborator

Huddie commented Oct 15, 2018

@j-f1 I’m not a fan of having code format every time I hit save tbh. i try to save often and code shifting each time can just feel weird.
Again, programming styles differ and no need for the code to auto-format each and every save. Feels redundant and also just messes with programming “flow?”.
I’d be more on board with a linter than forcing format each save. Thoughts?

Sent with GitHawk

@Huddie
Copy link
Collaborator

Huddie commented Oct 16, 2018

@rnystrom regardless of swiftLint vs swiftFormat, I think it's important we get a https://github.com/danger/danger file in the project.

I was going to add the file but it requires a bot, and only you can set that up

Reasons:

  • If swiftFormat occurs in the cloud
    • Easier with danger and allows for reporting back
  • If swiftLint
    • We need a danger file to report the formatting issues when PR are created. Simply having swiftLint in the project does not make PR review easier because you can't see the lint warnings.

This will also be beneficial for advancing PR Reviews in other aspects

Side Point: Not sure PR thread is the best place to be discussing this :p

@rnystrom
Copy link
Member Author

Totally down with adding danger!

Sent with GitHawk

@Huddie
Copy link
Collaborator

Huddie commented Oct 16, 2018

@rnystrom Thanks! I can set up the danger file with the linter/formatter once the dangerfile is in the repo

Sent with GitHawk

@BasThomas
Copy link
Collaborator

@Huddie @rnystrom fyi we have a ticket open to add Peril, which would mean we wouldn't need Danger: #1681

@Huddie
Copy link
Collaborator

Huddie commented Oct 16, 2018

Oh well if you go with Peril I don’t think I can help out. But I’m excited to see it in action! @BasThomas Peril is server side Danger right?

Sent with GitHawk

@BasThomas
Copy link
Collaborator

Yep!

@Huddie
Copy link
Collaborator

Huddie commented Oct 16, 2018

We have a server? Or it runs or GitHub or something?

Sent with GitHawk

@BasThomas
Copy link
Collaborator

It would require a server, but can be hosted on something like Heroku. See their setup guide.

@rnystrom
Copy link
Member Author

Alrighty, making a call to land this and start using SwiftLint so we can at least start having some standards in place. If we want to swap to SwiftFormat, that's fine too. Want to at least get this in before conflicts become a nightmare!

@rnystrom rnystrom merged commit c23945a into master Oct 17, 2018
@rnystrom rnystrom deleted the swiftlint branch October 17, 2018 01:55
@Huddie
Copy link
Collaborator

Huddie commented Oct 17, 2018

Sounds 😎

Sent with GitHawk

rnystrom added a commit that referenced this pull request Nov 2, 2018
* Fix cell size on the Bookmarks tab (#2124)

* Create code of conduct (#2126)

It is encouraged by GitHub to comply with recommended community standards.

* Fixed s/nuber/number/ typo in SECURITY.md (#2130)

* Add issue template for feature request (#2132)

Use GitHub template.

* Add issue template for bug report (#2131)

Use GitHub template.

* bump version to 1.23 (#2143)

* Replace "Inbox Zero" functionality and remove Firebase (#2142)

* simpler inbox zero date that allows date planning

* clean up

* remove firebase

* Don't override entire message when replying (#2160)

* Local push notifications (#2145)

* add fmdb

* add local notification cache mechanism

* rewiring to update local db when fetching notifications

* local pushes working

* building for xcode 10

*  Add UIAppearance styling for UISwitch & UISearchBar (#2144)

* Add(UIAppearance styling for UISwitch)

* Add(UIAppearance for UISearchBar)

* Added activity indicator (#2157)

* Fixes bug where send button is enabled after sending a comment (#2158)

* Load Consistency (#2159)

* Combine 2 load cells into 1

* Fixed load more in BaseViewController

* Switch if-else to ternary

* adds review GitHubAccess button to notifications view controller (#2176)

* Action Controller images (#2135)

* Add images to action controller

* Added pod to podfile
Added icon to NotificationSectionController

* Installed Pod

* ContextMenu Dominant corner

* Target support files

* Fixed support

* Updated Action Image Controller

* grammatical change to code signing instructions (#2186)

* Add TestFlight link to README (#2194)

* Add TestFlight link to README

* Update README.md

* fix double tap tab invalid (#2192)

this bug is caused by the time interval between two taps which is too small.

* Fixed color issue. (#2188)

* allows setting Milestone's loading indicator's color  (#2195)

* allows setting loading indicator's color

* updated spinner color to .white in Milestones/Labels/People

* Merge button status (#2189)

* Addresses #2178
Shows merge status without button when you cannot merge.

* Swift naming

* Fortify local notifications code (#2191)

* move path/defaults to be injected

* remove unused defaults

* add tests for notification db, fix settings, avoid dupe inbox requests

* better tests, fix dupe insert bug

* Attempt to fix iTunes Connect issue where watchOS needs min target (#2173)

* attempt to fix iTunes Connect issue where watchOS needs min target

* fix podfile

* Use updatedAt time to key notification send times (#2201)

* Use updatedAt time to key notification send times

* better formatting and fix tests

* Delete CNAME

* move blog to own repo (#2205)

* fixed icon name (#2203)

* Move lock (#2206)

* Moves lock under divider

* fix param style

* Update IssueManagingContextController.swift

* Switched to constants (#2207)

* move designs to own repo (#2208)

* Delete appcenter-post-clone.sh

* Delete AppStore.md

* Restore readme images after assets move (#2210)

* Move entitlements to Resources (#2209)

* Fixed cell border on merge button (#2204)

* Fixed cell border on merge button

* Fixed nits

* Settings cell textColor (#2193)

* Set API Status cell textColor to custom (Matching the rest of the settings page)

* Updated Double Tap Reaction off color to custom

* Updated Enabled textColor to custom

* Updated reaction textColors to proper custom

* Switched all labels to settingsLabels

* Fix entitlements path (#2211)

* change repo branch (#2202)

* change repo branch

additions:
- A change-repository-branch workflow thats very similar to the workflow in Milestones
- GraphQL query to fetch a repo's branches

modifications:
- a mutable "branch" string in RepositoryOverviewController and RepositoryCodeDirectoryViewController
- updated the V3RepositoryReadME fetch to include a branch parameter
- a protocol RepositoryBranchUpdatable to flag and update the appropriate ViewControllers when a user switches branches
- ContextMenu and UIAlertAction setup
- Removed some outdated code to get a repo's branch name from fetch(page:) in RepositoryOverviewViewController
- Added a line feed.adapter.reloadData() in fetch(page:) because app was crashing intermittently after a user switched branches without that line - was always an IGListKit duplicate identifier error on StyledTextRenderers  -  to reproduce, remove the feed.adapter.reloadData(), go to https://github.com/TheAlgorithms/Python, try to switch branches and the app will crash. I'm working raising an issue for it.

* requested changes

- changed public var to private(set) for var branch: String
- removed wasteful feed.adapter.reloadData() call
- re-formated params in switchBranchAction()

* style nit

* Localize Inbox Zero and allow for per-year holidays (#2215)

* Update Setup.md (#2218)

Grammatical change to md

* set spinner color in RepoBranchVC (#2221)

* Order repo branches after fetch (#2228)

* Order repo branches after fetch
- added func to order branches + unit tests
- replaced 'var branch: String' w/ 'private(set) var selectedBranch: String'
& added a defaultBranch variable
- changed fetch(page:) to update(animated:) in didSelect(value:) - an oversight from original pr

* AX animation

* Fix readme duplicate identifier asserts (#2219)

* WIP to fix readme asserts

* remove newline

* improved markdown parsing working

* fix tests

* Huge refactor of AppDelegate and authentication routing (#2238)

* refactor with new app controller, replace root nav mgr

* finish refactoring out root nav mgr

* remove root mgr

* Adds more share actions when browsing a repository (#2161) (#2237)

* add additional share actions

* fix share URLs

* use edge inset to right align image (#2232)

* new routing library and refactor shortcuts (#2241)

* [FIX] Settings write review indent (#2242)

* PeopleVC: Sort users with self first (#2246)

- moves sorting logic from fetch(page:) into type method
- unit test

* Simpler logic for review access button handling (#2249)

* Always update shortcuts on app launch (#2248)

* move the bookmark icon for the RepoVC and address a CR comment (#2255)

* always call completion block when early returning b/c of bg (#2258)

* open issue from notification (#2259)

* [FIX] Navigation for double tap reaction on settings (#2275)

- Changes segue kind from show to showDetail
- Adds a navigation controller for "Double tap Reaction" so it will work on both iPhone and iPad

* [FIX] Navigation controller on "View Source" from settings (#2276)

* keep read layer in front when animating inbox (#2280)

* Proper fix to prevent double fetching from background (#2279)

* Update Gemfile to use same version of CocoaPods referenced in Podfile.lock (#2277)

* Update cocoapods version constraint

* Run `bundle update cocoapods`

* Run `npm install`

* Add new iPhone models to UIDevice+Model.swift (#2278)

* Switched from long press to tap recognizer for menu controllers (#2271)

* Add steps in the setup to avoid commiting env variables to the repo (#2262)

* Add "Try Again" button for EmptyView (#2214) (#2226)

* Button to display Push Notification info in Settings (#2282)

* Add push notification info in settings

* better sizing

* add empty error view to latest base VC (#2283)

* move background handling into feed for better state control (#2285)

* thread error descriptions to squawk throughout app (#2286)

* Update MessageViewController (#2287)

* update messageviewcontroller

* add latest update

* update with revert

* Add clear button to action menus (#2288)

* Add clear button to the labels and milestone action menu

* Move the selection count in the PeopleViewController into the title
Add a clear button to the people controller

* Remove mock return for testing action menu

* Add "Clear" string constant
Remove unused Protocol

* Setup the clear enabled state and the Poeple title correctly on init

* Sorts issues after fetch (#2304)

*reverse chronologically

* Warn that logging out removes bookmarks (#2303)

* Enable SwiftLint and fix issues (#2292)

* update swiftlint

* build with lint enabled

* fix almost all warnings

* remove wholemodule

* fetch subscription status and fix mutation (#2291)

* update ContextMenu (#2310)

* reopen menu item is green (#2311)

* fix landscape read animation issue (#2312)

* Search for labels in app (take two) (#2314)

* Created DidTap Protocol for label section controllers

- created a protocol to route tap events to a delegate. The idea is that the delgate will have a reference to GithubClient and we can limit the baggage of passing around GithubClient

* Refactor Repo VC and SC to use name and owner as parameters

- Will make it easier to present RepoVC without needing a ref to RepositoryDetails. The only parameters being used from RepositoryDetails were owner and name

* Added presentLabels extension to UIViewController

* requested changes

* Show PR CI status inline with title (#2325)

* remove unused gql

* show PR CI status in list

* update StyledTextKit

* Thread networking errors and add custom error descriptions (#2324)

* Better network error descriptions

* localized descriptions for custom errors

* Improve empty retry UX (#2323)

* Highlight CodeView text on a background queue (#2322)

* Browse commit history of repositories, directories, and files (#2321)

* add history request

* Browse commit history of repo, directories, and files

* Move routes to own pod (#2317)

* Use https url for login (#2327)

Removes a deprecation warning for SFAuthentication because of using http

* Expand push settings info accessibility label (#2326)

* cleans up issue labels query (#2334)

* Move dropdown control to own lib (#2335)

* Move dropdown control to own lib

* update to add missing assets

* fix interaction, set appearances

* clear push notifications on open (#2336)

* update dropdown view for centered title view (#2338)

* inset preview collection view (#2340)

* update routing lib and wire up new repo route (#2344)

* Update reivewGitHUbAccessBtn constraints (#2339)

* [ADD] "Try Beta" cell on Settings (#2346)

Adds "Try Beta" cell on Settings screen, On tap opens the testflight invite link in safari

* Message on "Try Beta"  when already in TF (#2351)

* [ADD] On Tap "Try Beta", show Squawk message when user is already on beta

* Update Squawk+GitHawk.swift

* Fixes regex for consecutive shortlinks (#2358)

- updated unit tests

* Fix typo in TF squawk (#2361)

* Lowercase issue/PR search string (#2360)

* fix lints

* lowercase search string

* fix more lints

* Allow showing/cloning repository in Working Copy when installed (#2366)

.../more button when looking at repository has extra action when Working Copy is installed
to show or clone this repository in this app.

* Fixes Reaction emoji updates on split view (#2359)

* [FIX] Reaction emoji updates on split view

* [FIX] Change listener to delegate

* Update MessageViewController (#2370)

* Update routes to final form (#2374)

* update routes to final form

* fix routes build and update with router object
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants