Skip to content

Conversation

@danielrbrowne
Copy link
Contributor

@danielrbrowne danielrbrowne commented Feb 11, 2021

This PR:

Aims to improve the readability of the private API of the SDK, by simplifying it and bringing it more inline with Swift best practices. This introduces no major breaking changes to the public API, but can be seen as a precursor for the same kind of improvements being made to the public API for a future 10.0.0 release.

  • Renames some types in the private API (I.e. By removing the redundant Pusher prefix and/or using more descriptive nouns)
  • Consolidates the private cryptography functionality into the Crypto type
  • Moves some protocol definitions to their own source files (for consistency)
  • Ensures the Constants.API enum is not exposed publicly
    • This was a mistake when introduced in the 9.0.0 release, and it was not meant to be part of the public API. Technically this is a breaking change, however there is no end-user utility to exposing this enum so I consider this appropriate for a 9.x release.
    • A non-breaking API change adding a defaultHost to PusherHost was made to allow for this change.
  • Moves the private WebSocket URL constructor method to an extension method for the URL Foundation type
    • Previously, this was implemented as a global private method
  • Removes redundant public and open access level specifiers in Tests
    • There is no need to expose any functionality outside of the Tests target, as it is self-contained

@codecov-io
Copy link

codecov-io commented Feb 11, 2021

Codecov Report

Merging #339 (f6eb619) into master (18570cb) will decrease coverage by 0.03%.
The diff coverage is 69.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #339      +/-   ##
==========================================
- Coverage   89.46%   89.42%   -0.04%     
==========================================
  Files          50       50              
  Lines        3550     3538      -12     
==========================================
- Hits         3176     3164      -12     
  Misses        374      374              
Impacted Files Coverage Δ
Sources/Helpers/Logger.swift 53.84% <ø> (ø)
Sources/Models/ChannelsProtocolCloseCode.swift 83.33% <ø> (ø)
Sources/Models/PusherClientOptions.swift 100.00% <ø> (ø)
Sources/Models/PusherPresenceChannel.swift 82.81% <0.00%> (ø)
Sources/ObjC/PusherClientOptions+ObjectiveC.swift 0.00% <ø> (ø)
Sources/ObjC/PusherHost+ObjectiveC.swift 0.00% <0.00%> (ø)
Sources/Helpers/EventParser.swift 73.91% <33.33%> (ø)
Sources/Models/PusherChannel.swift 92.75% <33.33%> (ø)
Sources/Services/PusherConnection.swift 79.22% <45.28%> (ø)
...xtensions/PusherConnection+WebsocketDelegate.swift 63.28% <50.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18570cb...f6eb619. Read the comment docs.

@danielrbrowne danielrbrowne force-pushed the tech/refactor-private-api branch from 8e6d5fd to 579d71a Compare February 12, 2021 12:18
Copy link
Contributor

@TomKemp TomKemp left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Left some comments that don't require changes.

@danielrbrowne danielrbrowne merged commit 13fb1b6 into master Mar 1, 2021
@danielrbrowne danielrbrowne deleted the tech/refactor-private-api branch March 1, 2021 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants