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

remove endpoint generics #1529

Merged
merged 4 commits into from Jan 27, 2018

Conversation

Projects
None yet
9 participants
@zhongwuzw
Member

zhongwuzw commented Jan 6, 2018

Fixes #1524

@codecov-io

This comment has been minimized.

codecov-io commented Jan 6, 2018

Codecov Report

Merging #1529 into development will increase coverage by 1.96%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1529      +/-   ##
===============================================
+ Coverage        85.85%   87.82%   +1.96%     
===============================================
  Files               25        5      -20     
  Lines              834      156     -678     
===============================================
- Hits               716      137     -579     
+ Misses             118       19      -99
Impacted Files Coverage Δ
Sources/Moya/MultiTarget.swift
Sources/Moya/MultipartFormData.swift
Sources/Moya/Plugins/NetworkLoggerPlugin.swift
Sources/Moya/Plugins/CredentialsPlugin.swift
Sources/Moya/URL+Moya.swift
Sources/Moya/MoyaError.swift
Sources/Moya/Plugins/NetworkActivityPlugin.swift
Sources/Moya/TargetType.swift
Sources/Moya/Plugin.swift
Sources/Moya/AnyEncodable.swift
... and 6 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 6f16f6c...c472b2f. Read the comment docs.

@SD10

This comment has been minimized.

Member

SD10 commented Jan 6, 2018

Thanks for submitting this @zhongwuzw 👍 It seems like a good idea to have @Moya/contributors review this one.

@freak4pc

This comment has been minimized.

Member

freak4pc commented Jan 6, 2018

@zhongwuzw What problem does this PR solve, except for the "pure" aspect of that Concrete class not necessarily needing Target? There might be other developers who count on expecting Endpoints of certain Targets as inputs of classes, etc. Seems like a bit of a breaking change for no real benefit, unless I'm missing out.

I believe the original author of that class would be @ashfurrow - if you have a few seconds to say what you think about this, and what was the original reasoning behind using a Generic here, that might be helpful :)

@ashfurrow

This comment has been minimized.

Member

ashfurrow commented Jan 6, 2018

Hmm, yeah I'm undecided here. I think there was once a reason to have the Endpoint type be generic but it's beyond me now. I've looked through Eidolon and can't find any use of the generics, and there's not any member in the Endpoint type that even references the generic.

So on philosophical grounds, I think this PR is good. However, it's hard to say what the impact could be for the larger community. Additionally, there's code churn to be concerned with; this is a big change and I don't want to give the community a bunch of work to do without having a concrete benefit in mind. I mean, it's not a lot of work to migrate, but it is work, and it is a breaking change, so we'd need to roll this into Moya 11.0.

So I'm kind of split on this. I like the idea of removing the generic for code hygiene reasons, but I worry about unintended impact and code churn. Hopefully others who are more active users of Moya than I am can chime in about how they feel about it. Hope that context helps.

@SD10

This comment has been minimized.

Member

SD10 commented Jan 6, 2018

I think the PR is good too. However, I can’t see any positive gains from doing so other than clean code. The generic is resolved at compile time so there shouldn’t be any performance boosts.

Sent with GitHawk

@SD10

This comment has been minimized.

Member

SD10 commented Jan 6, 2018

@ashfurrow How do you recommend Moya handles code churn going forward? I realize it's important given the maturity of the project, but Moya was also built over 4 years of an unstable Swift.

For example, we still have class methods on MoyaProvider because a generic type couldn't have a nested type.

Though important now, maybe this becomes even more important when Swift itself reaches ABI stability?

@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Jan 6, 2018

I see the value of this PR in terms of cleaner endpoint/endpointClosure, but as others stated I'm not sure it outweighs the cons from migration/relying on the generic part of an Endpoint. We would have to investigate it a bit more to see what really would happen after the flattening.

@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Jan 7, 2018

@SD10 , Aside Endpoint, Generics would increase build time. In this situation of Endpoint, we need two steps, one is to find wether we need generics in Endpoint in some situations(by now, I can not found, maybe it has but rarely), another step is to investigate that how we can handle this in these situations.

@sunshinejr , yep, I love clean code😊, if we just need in rarely situations, I think we should remove it, because it leads to many useless code, build time, and increase executable size because specialization if we not need generics at all.

@SD10

This comment has been minimized.

Member

SD10 commented Jan 9, 2018

@zhongwuzw I don't expect this to significantly impact build times, even on a large project using Moya, but it is relevant.

Honestly, the migration for this is trivial. That does not worry me in the slightest. I just want to figure out if it is possible for someone to be relying on the generic before removing it. We need to figure out if it has any meaning.

@SD10

This comment has been minimized.

Member

SD10 commented Jan 18, 2018

Ironically, I found an issue #434 Endpoint doesn't need to be generic by @ashfurrow 🤣

@ashfurrow

This comment has been minimized.

Member

ashfurrow commented Jan 18, 2018

That's pretty funny – looks like #434 was closed and merged into #556, but that discussion itself didn't lead to changes and we never reconsidered #434. After a year and a half, I'm glad we're finally wrapping that up!

@ashfurrow

@zhongwuzw thank you for your contribution!

Okay, so this PR looks 💯 from a technical perspective; it's just missing some docs. This PR needs a changelog entry with links to #1524 and #1529, a one-sentence migration guide, and an explanation that most code will "just work". Let's also add a note to the inline Endpoint documentation about this change, with the same migration guide. We should also open up issues to take care of updating all of our docs (including our docs in Chinese).

@SD10 it's been over a week, and no one objected during the issue discussion. I think we can probably merge once everything is 🍏?

@SD10

This comment has been minimized.

Member

SD10 commented Jan 18, 2018

@ashfurrow I've been keeping track of all the changes to the English docs that are not reflected in the Chinese docs in #1357. Last week I released Moya 11.0.0-beta.1, should this change be merged before the official release of 11.0?

@zhongwuzw How about I take care of the documentation Ash has requested since my native language is English and you can translate them into Chinese for me?

@ashfurrow

This comment has been minimized.

Member

ashfurrow commented Jan 19, 2018

If we already have a major release in the works, I would recommend prioritizing this change and shipping it in the next beta.

@zhongwuzw

This comment has been minimized.

Member

zhongwuzw commented Jan 19, 2018

@SD10 , 💯 I'll updated Chinese docs after you updated the English docs.

@AndrewSB

I'm happy your hunch worked out 😄

@BasThomas

Just an fyi that we would have to update the documentation as well.

Already pointed out by @ashfurrow, sorry.

@SD10

This comment has been minimized.

Member

SD10 commented Jan 25, 2018

I got it @BasThomas. So sorry, I will really do this today.

  • CHANGELOG
  • Docs
  • Migration Guide
  • Inline Endpoint Docs

Sent with GitHawk

@BasThomas

This comment has been minimized.

Contributor

BasThomas commented Jan 25, 2018

No pressure @SD10!

@SD10

This comment has been minimized.

Member

SD10 commented Jan 26, 2018

Alright, really really sorry for the delays. I completed the checklist I made above.

I updated all the occurrences of the Endpoint generic in the docs_CN directory as well @zhongwuzw. So there doesn't seem to be anything to translate to Chinese. We could use the docs/migration_10_to_11.md translated but that seems like it should be another issue.

@SD10

This comment has been minimized.

Member

SD10 commented Jan 26, 2018

We had a lot of input on this one. Let me know if everything looks good to merge and we can get this merged into 11.0.0 ⛵️

@sunshinejr

It should be good to go! We will test it in beta.2 build of Moya 11 and wait for the feedback. Thanks for all the work and discussion everyone! 🎉

@sunshinejr sunshinejr merged commit e1aa653 into Moya:development Jan 27, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@zhongwuzw zhongwuzw deleted the zhongwuzw:remove-endpoint-generics branch Jan 28, 2018

@SD10 SD10 referenced this pull request Feb 8, 2018

Merged

[Release] Moya 11.0 #1568

@Jeroenbb94

This comment has been minimized.

Jeroenbb94 commented Feb 8, 2018

We did use the generic on our project but just as @ashfurrow mentioned, we don't know the reason anymore. The migration from 10 to 11 was easily done and our unit test still work without changing them.

@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Feb 8, 2018

@Jeroenbb94 Thank you very much for this feedback! We didn't hear any in beta stage so each report is really valuable to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment