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

Await all futures #297

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vkammerer
Copy link

@vkammerer vkammerer commented Nov 5, 2021

Hello and thank you for the library,

The function createIconsFromArguments exposed in main.dart returns a Future, so users expect that the task will be fully done when the Future completes, but it currently isn't the case.

How to reproduce
I noticed this because I am using the plugin in a dart CLI application, as one of our build steps. I am calling exit(0) after the Future completes, which causes the plugin to not write images to disk.

await createIconsFromArguments([]);
// images have not been written to disk at this point
exit(0)
// application exits without images 

Cause
This bug is due to the fact that many Futures are currently not awaited, which is hard to spot because they are called from synchronous functions.

Fix
I have tried to spot all the Futures and to await them, but it could be that I forgot some.
I also added the unawaited_futures lint rule.

Tested
I have tested this change in our build process and it all works fine: images are written to disk when createIconsFromArguments completes.

Caveat / Question
I wonder if this could affect the overall performance of the current implementation, since functions will be executed sequentially, while they are maybe executed concurrently in the current implementation?

@black-rusuz
Copy link

Merge, please, I also use the build script, and it does not work :c
@vkammerer what about conflict resolution?

@vkammerer
Copy link
Author

@black-rusuz you are the first person commenting on this PR since its creation over year ago. The maintainer hasn't commented or expressed interest in it, so I don't think it would be relevant for me to spend time resolving conflicts.

@black-rusuz
Copy link

black-rusuz commented Mar 2, 2023

@vkammerer Sounds logical, I didn't look at the publication date... Although, I find it strange that the developers of such a popular package, which is obviously good for CI/CD, didn't take care of it. Btw, now I'm using icons_launcher

@MarkOSullivan94
Copy link
Collaborator

@vkammerer apologies, I thought I had replied to this in the past, really appreciate this submission and definitely feel this is a good idea to include.

I also love how you've included unawaited_futures in the analysis_options.yaml too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants