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

Assets: improve colors performance #589

Merged
merged 3 commits into from Feb 8, 2019
Merged

Conversation

djbe
Copy link
Member

@djbe djbe commented Feb 4, 2019

Fixes #578.

To use lazy var, had to switch to final class with our own init. This is better than parsing the color during init as we can keep the @availability annotation for the color property.

@djbe djbe added this to the 6.1.1 milestone Feb 4, 2019
@SwiftGen-Eve
Copy link

SwiftGen-Eve commented Feb 4, 2019

Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!


Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

@djbe djbe force-pushed the fix/colors-template-performance branch from a16aeec to 6c229e9 Compare February 5, 2019 00:43
@djbe
Copy link
Member Author

djbe commented Feb 5, 2019

Rebased with changes from feedback.

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

LGTM but I'd love for @eaigner to weight in and confirm it fixes the performance issue they observed.

@eaigner
Copy link

eaigner commented Feb 5, 2019

While I can't speak to the performance aspect right now (the code was changed since i reported the issue), the stencil works.

@djbe djbe force-pushed the fix/colors-template-performance branch from 6c229e9 to 7598811 Compare February 5, 2019 09:57
@djbe
Copy link
Member Author

djbe commented Feb 5, 2019

Could you try it with your local generated code, replacing the struct with a final class, and the computed var by a lazy var?

@eaigner
Copy link

eaigner commented Feb 5, 2019

I have. But I did not benchmark performance. But since its lazy now, should not be a problem, right?

@djbe djbe force-pushed the fix/colors-template-performance branch 2 times, most recently from c9750d6 to e827d57 Compare February 6, 2019 12:59
@djbe
Copy link
Member Author

djbe commented Feb 7, 2019

I think it's safe to assume that the lazy vars solve the issue. Will merge this PR this evening unless there's more feedback.

@djbe djbe force-pushed the fix/colors-template-performance branch from e827d57 to 13da009 Compare February 8, 2019 19:26
@djbe djbe merged commit 59564dc into develop Feb 8, 2019
@djbe djbe deleted the fix/colors-template-performance branch February 8, 2019 19:44
@eaigner
Copy link

eaigner commented Mar 27, 2019

When is 6.2 scheduled to release? Would be nice to have this out there. It's almost 2 months now.

@djbe
Copy link
Member Author

djbe commented Mar 27, 2019

I've been swamped with work these last few weeks, big upcoming release at work.

TBH, things are ready for release, just some final checks and changelog/docs updates (see #621). Maybe this weekend, but I can't promise anything.

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.

None yet

4 participants