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
Nullsafety #851
Nullsafety #851
Conversation
Thanks - will take a look this week |
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.
I think, these things might need to get changed. Didn't take a very close look at everything, though
pubspec.yaml
Outdated
async: ^2.1.0 | ||
flutter_image: ^3.0.0 | ||
vector_math: ^2.0.0 | ||
flutter_image: |
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.
I think, the git:
part should be declared using a dependency_override
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.
Why?
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.
I don't know exactly what the convention is for this repositoy, but I think, it might be better to show that this is a temporary workaround until the changes are uploaded to pub
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.
I see it differently. A dependency_override
can easily be missed to be removed when the time comes. Also we don't have to override the dependency of an indirect dependency in this case
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.
I do think it's easier to remove dependency overrides when it's time to use the published version.
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.
ok, I moved it to dependency_override, but as the future version isn't available yet, I set the dependency entry to any
var downZoom = _scales.indexOf(downScale); | ||
// Check if scale is downScale => return array index | ||
if (scale == downScale) { | ||
return downZoom; | ||
} | ||
if (downScale == null) { | ||
return double.negativeInfinity; | ||
} | ||
// Interpolate | ||
var nextZoom = downZoom + 1; | ||
var nextScale = _scales[nextZoom]; |
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.
Is there any cases where nextZoom
can be out of _scales
boundaries ?
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.
Honestly I'm not deep enough into this part to tell. I deleted the three lines above.
Co-authored-by: Sata51 <vm.mathieu@gmail.com>
Co-authored-by: Sata51 <vm.mathieu@gmail.com>
Co-authored-by: Sata51 <vm.mathieu@gmail.com>
…tLng>' of 'stream'
Tile layer nullsafety
It looks like once https://github.com/flutter/flutter_image/pull/22 is published this will be ready |
Fix polyline border drawing
Hi @escamoteur - you need to rebase or merge you branch with current head of master to resolve current conflicts |
Sorry but I think this is a bit the wrong way that we continue to commit to master instead to this PR |
Do you mean a "code-freeze" on flutter_map master to reduce contention on your branch? I guess that could work for a short-lived branch. In this case however, we are waiting for external dependencies, which could take some time to be resolved (flutter_image is apparently not actively maintained at the moment). And since only you have the permission to push changes to your branch, it effectively blocks the community to maintain flutter_map until this PR is merged. What do you think @johnpryan? |
Actually I have to inform you that there is a lot work going on on flutter_image. A better alternative would probably by merge my branch into a null-safety branch on this repo so that all can work on it. |
Yes, that would be better. |
Who can do create a new branch and merge my PR there? |
Good to know. It does not look like that on github or pub.dev though. I left a comment to one of the product manages of Flutter at google asking them if they have time to look at the null-safety PR. Hopefully, he will update us on any plans for this PR |
I can |
Haven't you seen the comments on the PR from 2 days ago? |
I did, but it wasn't clear from Stuarts commenting (and profile, he is not a listed contributor to flutter_image) if he was in a position to actually decide (all are allowed to add a review) and merge the change. I did get an answer from mit-mit just now, indication that they are monitoring the PR. Hopefully, this means we can expect it to be merged soon. |
@escamoteur, branch issues-829-nullsafety is created. |
then we can close this PR? |
Not yet. I haven't merged your branch to it yet. Will do so soon. |
how is the status here? |
I'm Sorry @escamoteur , I've been swamped with work lately. The test was failing and had no time to analyse and fix it until now. Your contribution is available in branch issues/829-nullsafety. The new PR is #870. This PR can be closed. |
This is a migration to unsound null safety. So far I couldn't see problems, so I expect as soon as the missing packages are available that should be fine.
At some places I wasn't sure if previous nullable values should still be nullable, I left the null checks despite having changed the types to nonnullables and added a
TODO
comment.