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

Add target type in output of network logger plugin #1866

Conversation

@hasankose
Copy link

commented Jun 3, 2019

It provides print logs according to TargetType.
Fixes #1853

sunshinejr and others added some commits May 22, 2019

Merge pull request #1862 from Moya/sunshinejr-patch-1
Update Releasing.md with few additional steps to "After release" paragraph.
@MoyaBot

This comment has been minimized.

Copy link

commented Jun 3, 2019

1 Warning
⚠️ Consider also updating the Chinese docs. For Chinese translations, request the modified file(s) to be added to the list here for someone else to translate, if you can’t do so yourself.
3 Messages
📖 iOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.253 (13.417) seconds
📖 tvOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.232 (13.403) seconds
📖 macOS: Executed 278 tests, with 0 failures (0 unexpected) in 13.260 (13.439) seconds

Generated by 🚫 Danger

@sunshinejr
Copy link
Member

left a comment

Hey, this looks very good (and a lot simpler than I though it would be!) - thanks @hasan-monument! Would you please add a changelog entry (this will be a breaking change) and check any docs we have for the plugin and update them as well? Would be awesome!

@hasankose

This comment has been minimized.

Copy link
Author

commented Jun 4, 2019

Let me give a quick update, I've checked the TODOs below.

  • CHANGELOG.md is updated.
  • NetworkActivityPlugin and CredentialsPlugin have the TargetType in the closure.
  • TargetType is not needed for AccessTokenPlugin.
  • The Output which I've changed is not used in the Plugins doc and it is not needed to update(I can add an example if needed).
@codecov-io

This comment has been minimized.

Copy link

commented Jun 4, 2019

Codecov Report

Merging #1866 into development will not change coverage.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #1866   +/-   ##
============================================
  Coverage        92.13%   92.13%           
============================================
  Files               25       25           
  Lines              865      865           
============================================
  Hits               797      797           
  Misses              68       68
Impacted Files Coverage Δ
Sources/Moya/Plugins/NetworkLoggerPlugin.swift 93.44% <77.77%> (ø) ⬆️

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 cf9ee28...5fead2d. Read the comment docs.

@pedrovereza

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@sunshinejr @hasankose Hey, how about we make target an optional parameter? This will keep the plugin working as it used to, and also enable users to filter by TargetType when they need.

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@pedrovereza is it possible to do so without making a breaking change? Even if there is an optional parameter, you’d still need to handle it in the closure, right?

@pedrovereza

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@sunshinejr Ah, you're right! Ignore me, I miss understood how the changes were being added here 😅

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@pedrovereza no worries, this was something I didn't consider before so good question!

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@hasankose alright I've pushed a small polish to the Changelog entry but we're good to go! Thanks again for looking into this, good timing with the new major release as well 👍

@sunshinejr sunshinejr merged commit bbe5d2c into Moya:development Jun 4, 2019

0 of 2 checks passed

ci/circleci: build Your tests are queued behind your running builds
Details
ci/circleci: carthage_without_swiftlint_integration CircleCI is running your tests
Details
@peril-moya

This comment has been minimized.

Copy link

commented Jun 4, 2019

@hasankose Thanks a lot for contributing to Moya! We've invited you to join
the Moya GitHub organization – no pressure to accept! If you'd like more
information on what that means, check out our contributor guidelines and
feel free to reach out to @Moya/core-team with any questions.

Generated by 🚫 dangerJS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.