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.

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

Exclude build folder from Swiftlint #5412

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Nov 9, 2023


This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Nov 9, 2023
Copy link

linear bot commented Nov 9, 2023

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Also includes some refactoring to remove lint errors.

Reviewable status: 0 of 4 files reviewed, all discussions resolved

@rablador rablador force-pushed the exclude-build-folder-from-swiftlint-ios-382 branch from bf2aac9 to 960d99e Compare November 9, 2023 16:15
Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/AppDelegate.swift line 118 at r1 (raw file):

        addressCache.loadFromFile()

        setUpProxies(containerURL: containerURL)

It's hard enough to follow what's going on. Perhaps it would be easier if the call to setUpProxies() wouldn't be nested in setUpCaches() and instead would follow the call to setUpCaches()?


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 334 at r1 (raw file):

    /**
     Create a connection state.

connectedState() doesn't describe particularly well what this method does. It's not even used when the actor is in connected state. The documentation for this method does not describe it either.

Since this method is only used when in initial or error state, it seems that this call is a precursor to [re]connecting state rather than connected state.

It would be more appropriate to call it something along the lines that produces a connection state for connection attempt. Or maybe simply mark it as makeConnectionStateInner() and document what it does.


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 337 at r1 (raw file):

     - Parameters:
     - nextRelay: next relay to connect to.

Nit: Parameters listed under the "- Parameters:" should be indented with two spaces, I think... Xcode has a bug where it doesn't do that for block comments but it does that for /// comments.

For example:

- Parameters:
  - paramA: <description>
  - paramB: <description> 

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador)

@rablador rablador force-pushed the exclude-build-folder-from-swiftlint-ios-382 branch from 960d99e to 4369ccb Compare November 13, 2023 12:37
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pronebird)


ios/MullvadVPN/AppDelegate.swift line 118 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

It's hard enough to follow what's going on. Perhaps it would be easier if the call to setUpProxies() wouldn't be nested in setUpCaches() and instead would follow the call to setUpCaches()?

I cannot follow the call since setUpProxies() creates the apiProxy that's used inside setUpCaches(). And neither can it precede it. I simply reverted this part and created a smaller refactor of TunnelManager creation intead. I'm not looking for real improvement here, mostly just fix the swiftlint error.


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 334 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

connectedState() doesn't describe particularly well what this method does. It's not even used when the actor is in connected state. The documentation for this method does not describe it either.

Since this method is only used when in initial or error state, it seems that this call is a precursor to [re]connecting state rather than connected state.

It would be more appropriate to call it something along the lines that produces a connection state for connection attempt. Or maybe simply mark it as makeConnectionStateInner() and document what it does.

Done.


ios/PacketTunnelCore/Actor/PacketTunnelActor.swift line 337 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Nit: Parameters listed under the "- Parameters:" should be indented with two spaces, I think... Xcode has a bug where it doesn't do that for block comments but it does that for /// comments.

For example:

- Parameters:
  - paramA: <description>
  - paramB: <description> 

The rest of the file uses four spaces for comments, so I changed to that.

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rablador rablador force-pushed the exclude-build-folder-from-swiftlint-ios-382 branch from 4369ccb to 220c6cc Compare November 14, 2023 08:18
@rablador rablador force-pushed the exclude-build-folder-from-swiftlint-ios-382 branch from 220c6cc to 35b51fe Compare November 14, 2023 08:24
@buggmagnet buggmagnet requested a review from pronebird November 14, 2023 08:47
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1, 1 of 2 files at r3.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @pronebird)

@buggmagnet buggmagnet merged commit d57f06a into main Nov 14, 2023
@buggmagnet buggmagnet deleted the exclude-build-folder-from-swiftlint-ios-382 branch November 14, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants