chore: swift lint cleanup #756

JeneaVranceanu · 2023-02-02T16:09:52Z

Summary of Changes

Review only after #761

Manually fixing SwiftLint issues.

Test Data or Screenshots

By submitting this pull request, you are confirming the following:

I have reviewed the Contribution Guidelines.
I have performed a self-review of my own code.
I have updated my repository to match the develop branch.
I have included test data or screenshots that prove my fix is effective or that my feature works.
I have checked that all tests work and swiftlint is not throwing any errors/warnings.

* develop-upstream: chore: EIP681 docs + more descriptive log message fix: ENS parsing requires chainID if an ENS address is expected to be requested/decoded - we do not assume that client is using Ethereum Mainnet! fix: RLP func encode for single value has named parameter chore: test refactoring - checking for nullability by using XCTAssertNotNil chore: replaced [Any]() with [] for default parameter value fix: removed as AnyObject from ENS related code chore: removed spacing in `" )` fix: spacings chore: removed redundant `parameters: []` in read operation fix: EIP681 - when parsing arguments chain ID must be defaulted to Mainnet instead of 0 fix: Networks.fromInt must not return optional fix: some spacing and docs fixed fix: ERC20BaseProperties are back to inherit from AnyObject (restricting protocol to classes) fix: removed the use of AnyObject in all places possible - replaced with Any # Conflicts: # Tests/web3swiftTests/localTests/ABIEncoderTest.swift # Tests/web3swiftTests/localTests/TransactionsTests.swift

…ultiline function calls

JeneaVranceanu · 2023-02-03T14:57:43Z

.swiftlint.yml

  - type_name
  - identifier_name
  - line_length
  - multiple_closures_with_trailing_closure
  - todo
+  - indentation_width


Enabling this rule doesn't allow us to have multiline function calls like this one:

Perhaps we should use swiftlint --fix on this file to propose a solution that is acceptable to that tool. If that solution is also acceptable to project maintainers then we should go that way instead of disabling this rule. If all that succeeds then the code format will align with other swiftlinted projects.

This rule has no autocorrection. Warnings appear on completely valid pieces of code.

Update: found the reason for this warning. We should not place the first argument on the same line as the function name. The code should be instead:

Not an unreasonable variation of indentation and style.

But it also applies to guard statements (and likely not only). All conditions, if there are multiple, should take a separate line each and must not be on the same line as guard keyword:

This variation is also allowed by the rule, but it's less readable and not pleasant to look at at all:

Based on these findings I'll re-enable this rule per @cclauss suggestion.
@yaroslavyaroslav @janndriessen Please be aware that the examples above are now the style for multiline function calls and guard multiline statements.

This rule is all about indentation and is compatible with almost all code in web3swift.
It doesn't force us to write guard statements in multiline always. Here is one of the acceptable examples:

.swiftlint.yml

JeneaVranceanu · 2023-02-05T15:57:33Z

@cclauss Thanks for the suggestion, I should've done that right from the beginning.
Opened 2 new PRs with config file changes and with swiftlint --fix changes only.

yaroslavyaroslav · 2023-02-09T13:15:24Z

@JeneaVranceanu Not sure is it my fault, but the pr is broken so far, fix it please.

JeneaVranceanu · 2023-02-09T15:22:12Z

@yaroslavyaroslav Fixed.
But this has to be merged first -> #761

yaroslavyaroslav · 2023-02-09T17:10:47Z

Sources/Web3Core/KeystoreManager/EthereumKeystoreV3.swift

@@ -6,6 +6,7 @@
 import Foundation
 import CryptoSwift

+// swiftlint:disable cyclomatic_complexity


Shouldn't we enable it back at the end of the struct?

No, not in this case.
Basically whole file requires refactoring, and for now to silence the cyclomatic_complexity issues in this file I've added that comment.
The rule is automatically re-enabled when SwiftLint leaves this file and starts linting the other one.

yaroslavyaroslav

There's a several questions to those changes.

cclauss · 2023-02-09T21:02:02Z

Does this PR need a rebase?

JeneaVranceanu · 2023-02-09T22:46:09Z

Does this PR need a rebase?

@cclauss It's merged with develop branch.
There are still plenty of files that have changes but these are the result of things like replacement of {return nil} with { return nil }.

cclauss · 2023-02-09T23:12:43Z

Agreed. The whitespace-only changes like{ return nil } could moved to a separate pr that is trivial to review and land. This would leave just those few modifications that require more debate or rewriting or linter directives.

yaroslavyaroslav · 2023-02-10T05:51:45Z

@cclauss just wonder how much your into us? If you are, I can add you to a list of reviewers that would make your review countable in ci/cd checks and grant you rights to merge, should I?

janndriessen · 2023-02-10T07:47:40Z

Sources/web3swift/Web3/Web3+Utils.swift

+            case .Microether:
+                return 12
+            case .Finney:
+                return 15


…eam#756

…anges chore: swift lint cleanup -- whitespace-only changes from #756

JeneaVranceanu · 2023-02-10T11:00:53Z

@cclauss Thanks for the #773
This PR is updated now.

JeneaVranceanu added 5 commits February 2, 2023 17:20

chore: a few swiftlint rules to ignore for now

dfa960a

fix: EthereumKeystoreV3 ignore cyclomatic complexity

c5cf027

chore: swiftlint --fix

e90dfb2

chore: swiftlint --fix

c86697c

JeneaVranceanu mentioned this pull request Feb 2, 2023

Update workflow for swiftlint #690

Closed

4 tasks

JeneaVranceanu added 13 commits February 3, 2023 14:28

chore: merge with develop

a5f270d

chore: disable swiftlint indentation_width rule as it doesn't allow m…

f8f9f2a

…ultiline function calls

fix: swiftlint errors in SECP256K1.swift

8b37e6f

fix: swiftlint errors in Web3+Instance.swift

662e3bf

fix: swiftlint errors in Web3+Eventloop.swift

5dcb100

fix: swiftlint errors in Web3.swift

ded3fee

fix: swiftlint errors in Web3+InfuraProviders.swift

ae205a7

fix: swiftlint issues in Web3+Utils.swift

a5a528a

fix: commented_out_code rule regular expression

9f8f8fc

fix: swiftlint issues in Web3+EIP1559.swift

d3a427b

fix: swiftlint issues in ENSReverseRegistrar.swift

60560c3

fix: swiftlint issues in NameHash.swift

e96d426

fix: swiflint issues in ETHRegistrarController.swift

909b323

JeneaVranceanu commented Feb 3, 2023

View reviewed changes

.swiftlint.yml Outdated Show resolved Hide resolved

JeneaVranceanu commented Feb 3, 2023

View reviewed changes

.swiftlint.yml Outdated Show resolved Hide resolved

JeneaVranceanu added 8 commits February 3, 2023 17:14

fix: swiftlint commented_out_code rule fixes

78c8f6c

fix: removed swiflint:disable nested rule from Web3+Utils.swift

b18fbe7

fix: swiftlint issues in EIP4361.swift

ae4ba9e

fix: ERC1155 swiftlint fixes

4330304

chore: minor refactoring of guard statements

3ad9a6b

fix: ERC721 swiftlint

275d2cd

fix: ERC712 swiftlint

42bb154

chore: implemented EthereumAddress tests

ac05664

JeneaVranceanu mentioned this pull request Feb 5, 2023

chore: swiftlint auto fixes #761

Merged

JeneaVranceanu self-assigned this Feb 5, 2023

JeneaVranceanu added the refactoring label Feb 5, 2023

Merge branch 'chore/swiftlint-auto-fixes' into chore/swift-lint-cleanup

5b2884e

chore: merge with chore/swiftlint-auto-fixes

9f8f820

JeneaVranceanu added the ready for review issue is resolved, a final review is needed before closing label Feb 5, 2023

yaroslavyaroslav previously approved these changes Feb 9, 2023

View reviewed changes

chore: merge with develop branch

99c9b55

JeneaVranceanu dismissed yaroslavyaroslav’s stale review via 99c9b55 February 9, 2023 15:20

chore: merge with develop

b1c168d

yaroslavyaroslav reviewed Feb 9, 2023

View reviewed changes

janndriessen reviewed Feb 10, 2023

View reviewed changes

Sources/web3swift/Web3/Web3+Utils.swift

case .Microether:

return 12

case .Finney:

return 15

Copy link

Collaborator

janndriessen Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟

cclauss added a commit to cclauss/web3swift that referenced this pull request Feb 10, 2023

chore: swift lint cleanup -- whitespace-only changes from web3swift-t…

2d15167

…eam#756

cclauss mentioned this pull request Feb 10, 2023

chore: swift lint cleanup -- whitespace-only changes from #756 #773

Merged

JeneaVranceanu added a commit that referenced this pull request Feb 10, 2023

Merge pull request #773 from cclauss/swift-lint-cleanup-whitespace-ch…

9bd3042

…anges chore: swift lint cleanup -- whitespace-only changes from #756

chore: merged with develop

809d1d8

yaroslavyaroslav approved these changes Feb 10, 2023

View reviewed changes

cclauss approved these changes Feb 10, 2023

View reviewed changes

janndriessen approved these changes Feb 10, 2023

View reviewed changes

JeneaVranceanu merged commit bce743e into web3swift-team:develop Feb 10, 2023

JeneaVranceanu deleted the chore/swift-lint-cleanup branch February 10, 2023 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: swift lint cleanup #756

chore: swift lint cleanup #756

JeneaVranceanu commented Feb 2, 2023 •

edited

JeneaVranceanu Feb 3, 2023

cclauss Feb 4, 2023 •

edited

JeneaVranceanu Feb 5, 2023

JeneaVranceanu Feb 5, 2023

JeneaVranceanu commented Feb 5, 2023

yaroslavyaroslav commented Feb 9, 2023

JeneaVranceanu commented Feb 9, 2023

yaroslavyaroslav Feb 9, 2023

JeneaVranceanu Feb 9, 2023

yaroslavyaroslav left a comment

cclauss commented Feb 9, 2023

JeneaVranceanu commented Feb 9, 2023 •

edited

cclauss commented Feb 9, 2023

yaroslavyaroslav commented Feb 10, 2023

janndriessen Feb 10, 2023

JeneaVranceanu commented Feb 10, 2023

chore: swift lint cleanup #756

chore: swift lint cleanup #756

Conversation

JeneaVranceanu commented Feb 2, 2023 • edited

Summary of Changes

Test Data or Screenshots

By submitting this pull request, you are confirming the following:

JeneaVranceanu Feb 3, 2023

Choose a reason for hiding this comment

cclauss Feb 4, 2023 • edited

Choose a reason for hiding this comment

JeneaVranceanu Feb 5, 2023

Choose a reason for hiding this comment

JeneaVranceanu Feb 5, 2023

Choose a reason for hiding this comment

JeneaVranceanu commented Feb 5, 2023

yaroslavyaroslav commented Feb 9, 2023

JeneaVranceanu commented Feb 9, 2023

yaroslavyaroslav Feb 9, 2023

Choose a reason for hiding this comment

JeneaVranceanu Feb 9, 2023

Choose a reason for hiding this comment

yaroslavyaroslav left a comment

Choose a reason for hiding this comment

cclauss commented Feb 9, 2023

JeneaVranceanu commented Feb 9, 2023 • edited

cclauss commented Feb 9, 2023

yaroslavyaroslav commented Feb 10, 2023

janndriessen Feb 10, 2023

Choose a reason for hiding this comment

JeneaVranceanu commented Feb 10, 2023

JeneaVranceanu commented Feb 2, 2023 •

edited

cclauss Feb 4, 2023 •

edited

JeneaVranceanu commented Feb 9, 2023 •

edited