Skip to content

Add SwiftLinter #524

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

Closed
wants to merge 40 commits into from
Closed

Conversation

pharms-eth
Copy link
Collaborator

@pharms-eth pharms-eth commented Apr 17, 2022

add Swift Linter in a Xcodeproj file to start cleaning up each rule
disabled all default rules to re-add one by one

other changes in here from Async work, PR will simplify when other PR is merged

@pharms-eth pharms-eth marked this pull request as draft April 17, 2022 01:10
-- redundant_discardable_let
- redundant_objc_attribute
- redundant_optional_initialization
- redundant_set_access_control
- redundant_string_enum_value
- redundant_void_return
@mloit
Copy link
Contributor

mloit commented Apr 17, 2022

Couple of things. first we already have a .swiftlint.yml in the root of the repo, so if we're adding/changing rules it should be done there. Secondly you appear to have several updates to your swiftlint in the form of other PR's that look to be just commits against this PR. Since they are all relative to this one, I suggest combining them to this one, as this one is still 'in progress' until it is merged. You can keep making new commits against this one, and not need to open a new PR for every change.

we use a lot of empty enum arguments so kept disabled
we use the old block kvo
we have some protocols with delegate in the name
@pharms-eth
Copy link
Collaborator Author

the .swiftlint.yml file I currently have in here is for in place cleanup. When I open this PR (no longer draft) I will remove my working copy and update the project one

@mloit
Copy link
Contributor

mloit commented Apr 17, 2022

I see, okay. That's fine then.

Thank you for removing the 3 redundant PR's from yesterday, but 2 more popped up today. Please update your process, so you are not generating new PR's on what is really just work against PR. The PR will automatically track any commits made to the branch you created it from ([your-repo]/SwiftLint). If you make a branch from that to try something, all you need to do is merge it back into [your-repo]/SwiftLint and push, and the PR will track those changes.

@yaroslavyaroslav
Copy link
Collaborator

Please close the pr and merge changes made here into #515

@mloit mloit mentioned this pull request Apr 24, 2022
@yaroslavyaroslav
Copy link
Collaborator

Copy paste recent comment of @mloit here:

This PR may fix those. They are separate in the sense that this contains lint fixes across the entire repo, where that PR is focused on switching from PromiseKit to Async/Await. He has branched this PR from that one (actually this PR is branched from #524, which was branched from #515), so all the changes there are included here. I've refrained from reviewing this one until that one has been merged as it's impossible to otherwise separate the changes from which PR they are being made in.

In my opinion I think it best to keep them separate, and comment on any lint issues in the changes of that PR there. Then after that one is merged, this one (and the others he has) should clean up a bit. Though as they are still branched from each other it is impossible to separate the unique changes in this one, vs those made in #524. Personally I don't like this form of PR as we end up reviewing the same changes over and over again, and makes it very difficult to focus on the changes that the PR is actually supposed to be focused on.

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.

3 participants