-
Notifications
You must be signed in to change notification settings - Fork 235
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
Run on UiThread and few others #328
Conversation
@deakjahn What a massive update. I'll try to go over this soon. Could you kindly check the CI failing? |
The formatter exits with an error code 1 but without giving any meaningful error message. Hmmm. It runs for me locally. At any rate, this is the format you used earlier, so I'll upload with this new format now, there will be less difference compared to your current layout. |
Yep. Flutter formatting defaults to 80-char line lengths which I, frankly, hate. I consider this setting fundamentally flawed with today's monitor sizes but I witnessed and even took part in many disputes with Munificient, the one man driving force behind dart (now flutter) format and he cannot be convinced otherwise. Now as it's reverted, the changes are less numerous, although in quite a few places I did change from setting a variable and immediately returning it to returning it in one step. This makes it look like a really massive change but the actual, meaningful changes are less than that. Come to think of it, format had an |
Codecov Report
@@ Coverage Diff @@
## master #328 +/- ##
==========================================
+ Coverage 45.22% 45.97% +0.75%
==========================================
Files 3 3
Lines 429 435 +6
==========================================
+ Hits 194 200 +6
Misses 235 235
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me so far.
Let's dangerously release to production 🙏
@hyochan I switched to the published 5.1.1 in my code and it seems to work OK. :-) |
It was quite some time ago and I forgot what it was all about, now I can see. I don't get any error in my app but it might not use the very same services. Didn't the modification suggested by 0x-cell solve it? Because I can't seem to have the same error, I woudn't be able to reproduce it. |
@mgonzalezc I spoke too soon, I started receive something but very recently, only as of yesterday. Not the very same (null in line 59 of |
With reference to #272
So, a few words about the changes. First of all, sorry about some whitespace differences but, apparently, our IDEs use dartformat with slightly different settings.
The main difference is a new
MethodResultWrapper
class that wraps both the result and the channel.onMethodCall()
now immediately saves this wrapped result-channel to a field and only uses that later to set both the result and to send back info on the channel. I did this in both Google and Amazon but I can't test the Amazon one.Second, I included the plugin registration differences I suggested in the issue queue above.
Third, I did the modification suggested in one of the issues (I can't find it right now to link it, sorry) that
initConnection
,endConnection
andconsumeAllItems
shouldn't be accessors. This is very much so, property accessors are not supposed to do work and have side effects, just return a value. I suggested now three new functions and marked the old ones deprecated, so you could safely leave it as is for some time, maybe until the next major version, if you accept the suggestion.Fourth,
EnumUtil.getValueString()
is not really necessary, we havedescribeEnum()
in the Flutter engine just for this purpose.