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

Nullsafety #851

Closed
wants to merge 19 commits into from
Closed

Nullsafety #851

wants to merge 19 commits into from

Conversation

escamoteur
Copy link
Contributor

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.

@johnpryan
Copy link
Collaborator

Thanks - will take a look this week

Copy link
Contributor

@ThexXTURBOXx ThexXTURBOXx left a 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

.packages Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pubspec.yaml Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
pubspec.yaml Outdated
async: ^2.1.0
flutter_image: ^3.0.0
vector_math: ^2.0.0
flutter_image:
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

pubspec.yaml Outdated Show resolved Hide resolved
lib/src/core/bounds.dart Outdated Show resolved Hide resolved
@josxha josxha mentioned this pull request Mar 25, 2021
lib/src/core/point.dart Outdated Show resolved Hide resolved
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];
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

lib/src/gestures/latlng_tween.dart Outdated Show resolved Hide resolved
escamoteur and others added 6 commits March 25, 2021 15:56
Co-authored-by: Sata51 <vm.mathieu@gmail.com>
Co-authored-by: Sata51 <vm.mathieu@gmail.com>
Co-authored-by: Sata51 <vm.mathieu@gmail.com>
@johnpryan
Copy link
Collaborator

It looks like once https://github.com/flutter/flutter_image/pull/22 is published this will be ready

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

Hi @escamoteur - you need to rebase or merge you branch with current head of master to resolve current conflicts

@escamoteur
Copy link
Contributor Author

Sorry but I think this is a bit the wrong way that we continue to commit to master instead to this PR

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

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?

@escamoteur
Copy link
Contributor Author

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.

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

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.

@escamoteur
Copy link
Contributor Author

Who can do create a new branch and merge my PR there?

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

Actually I have to inform you that there is a lot work going on on flutter_image.

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

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

Who can do create a new branch and merge my PR there?

I can

@escamoteur
Copy link
Contributor Author

Haven't you seen the comments on the PR from 2 days ago?

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

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.

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

@escamoteur, branch issues-829-nullsafety is created.

@escamoteur
Copy link
Contributor Author

then we can close this PR?

@kengu
Copy link
Contributor

kengu commented Apr 8, 2021

then we can close this PR?

Not yet. I haven't merged your branch to it yet. Will do so soon.

@escamoteur
Copy link
Contributor Author

how is the status here?

@kengu
Copy link
Contributor

kengu commented Apr 14, 2021

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.

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.

None yet

7 participants