search

fiskaly / fiskaly-sdk-swift

fiskaly Cloud-TSE SDK for Swift/iOS
MIT License
5 stars 5 forks source link

Feature/sdk refactor #17

Closed prempador closed 4 years ago

prempador commented 4 years ago

Motivation

Static Archive

In order to make the SDK easier to integrate we switched the client Framework to a static archive. With this change we also do not support armv7 anymore. Changes to integration of the SDK are reflected in the Example in this repository and in the readme.

Remove Completion Handlers

As addressed in #7 network requests in this SDK are performed synchronously. Therefore having completion-handlers was not useful and ended in confusion. As we have no plans to make these request async (there are different ways to touch this topic and we are not willing to force integrator to use a specific method) we removed the completion handlers. Following this, errors returned by the fiskaly Client are now thrown by the functions as Error and can be caught in catch blocks.

Remove Force Unwrapping

Mentioned in #9 force unwrapping was used in various places, what could cause the app of the integrator to crash. With this pull request all force unwraps should be removed.

Changes to Lint

With the Contribution Guidelines we also integrated a linter. As we had some implementation, not conform to Swift conventions, we adapted the linter to accept these cases. As we implemented breaking changes in this Pull Request we also changed the parameter of some functions to be conform to Swift conventions and adapted the linter to not accept these anymore. Effected are:

VersionResult: sourceHash, commitHash Config: debugLevel, debugFile, clientTimeout, smaersTimeout

(see a28c492)

Narrow down Responses

Responses as well as Errors are now narrowed down to the useful information, stripping Jsonrpc Error and Response Codes as well as the id from the requests to the fiskaly Client, as they are not used by the integrator and just make the responses not clean to use.

Test Plan

All existing tests were adapted to the changes, no new tests were added.

Side Notes

closes #7 closes #9

prempador commented 4 years ago

Hey @marcelvoss,

as there are some changes that are implemented to address your issues, would you mind taking a look at what I changed and giving feedback before I merge this into master? If you find any issues with these changes I could change it here before merging them.

marcelvoss commented 4 years ago

Hi @prempador,

thanks for the heads up - much appreciated. Will do until EOD, if that's fine for you.

prempador commented 4 years ago

thanks for the heads up - much appreciated. Will do until EOD, if that's fine for you.

Sure, tomorrow is holiday here so I will most likely adapt / merge on friday.

prempador commented 4 years ago

Thanks again @marcelvoss for the reviews and all the comments. I tried to address all of them as good as possible. The Errors are still not optimal and need some more work. As I have to prepare the newest release for the SDK I will merge this PR now, followed by your PR and changes to Errors will be addressed after the new release. If you have any more comments feel free to create a new issue!

Thanks for your time.