Skip to content

Nullsafety#851

Closed
escamoteur wants to merge 19 commits intofleaflet:masterfrom
escamoteur:nullsafety
Closed

Nullsafety#851
escamoteur wants to merge 19 commits intofleaflet:masterfrom
escamoteur:nullsafety

Conversation

@escamoteur
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

Thanks - will take a look this week

Copy link
Copy Markdown
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

Comment thread .packages Outdated
Comment thread CHANGELOG.md Outdated
Comment thread pubspec.yaml
Comment thread pubspec.yaml Outdated
Comment thread pubspec.yaml Outdated
async: ^2.1.0
flutter_image: ^3.0.0
vector_math: ^2.0.0
flutter_image:
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread pubspec.yaml Outdated
Comment thread lib/src/core/bounds.dart Outdated
@josxha josxha mentioned this pull request Mar 25, 2021
Comment thread lib/src/core/point.dart Outdated
Comment thread lib/src/geo/crs/crs.dart
}
// Interpolate
var nextZoom = downZoom + 1;
var nextScale = _scales[nextZoom];
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread lib/src/gestures/latlng_tween.dart Outdated
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
Copy Markdown
Collaborator

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

@kengu
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@kengu
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kengu commented Apr 8, 2021

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

I can

@escamoteur
Copy link
Copy Markdown
Contributor Author

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

@kengu
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kengu commented Apr 8, 2021

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

@escamoteur
Copy link
Copy Markdown
Contributor Author

then we can close this PR?

@kengu
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

how is the status here?

@kengu
Copy link
Copy Markdown
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.

7 participants