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.

Sign up for GitHub

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

Jump to bottom

Add Networking module #87

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Add Networking module #87

merged 3 commits into from
Jul 7, 2022

Conversation

dmikots
Copy link
Contributor

@dmikots dmikots commented Jul 6, 2022

#86
Add generic networking + add some service to open emoji api. No tests yet

@dmikots dmikots added the OS: all Related to all operating systems (macOS and iPadOS) label Jul 6, 2022
@dmikots dmikots requested a review from voltangle July 6, 2022 20:00
@voltangle voltangle added enhancement New feature or request internal Internal things labels Jul 6, 2022
@voltangle voltangle added this to the Stage 2: Media empire milestone Jul 6, 2022
@voltangle voltangle linked an issue Jul 6, 2022 that may be closed by this pull request
Add Networking module #86
Closed
voltangle
voltangle requested changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@voltangle voltangle left a comment

Choose a reason for hiding this comment

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

As a note, I would move EmojiModel, EmojiNetworking and EmojiService to the Networking module, so the entire thing is abstracted away

@dmikots dmikots requested a review from voltangle July 7, 2022 18:01
voltangle
voltangle reviewed Jul 7, 2022
View reviewed changes
Copy link
Collaborator

@voltangle voltangle left a comment

Choose a reason for hiding this comment

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

In general, it looks alright, but I would think that moving the networking to Alamofire will be much better, so we can write cleaner and more readable code

@dmikots dmikots requested a review from voltangle July 7, 2022 18:33
voltangle
voltangle approved these changes Jul 7, 2022
View reviewed changes
Copy link
Collaborator

@voltangle voltangle left a comment

Choose a reason for hiding this comment

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

All clear!

Well, you can also try incorporating Alamofire instead of URLSession, but that's not a requirement, that's a suggestion

If you can't/don't want to do it, then don't, I will do it eventually 😅

@voltangle voltangle changed the title some networking async Add Networking module Jul 7, 2022
@voltangle voltangle merged commit e51d289 into master Jul 7, 2022
@voltangle voltangle deleted the networking branch July 7, 2022 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voltangle voltangle voltangle approved these changes

Assignees

@dmikots dmikots

Labels
enhancement New feature or request internal Internal things OS: all Related to all operating systems (macOS and iPadOS)
Projects
None yet
Milestone
Stage 2: Media empire
Development

Successfully merging this pull request may close these issues.

Add Networking module
2 participants
@dmikots @voltangle