close2 / csv

A dart csv to list codec / converter
MIT License
98 stars 24 forks source link

Migration to Null-Safety #38

Closed darwingr closed 3 years ago

darwingr commented 3 years ago

Followed official migration guide here: https://dart.dev/null-safety/migration-guide

Was looking to upgrade my own project and this was the only package dependency that didn't have null safety yet. I did my best to not change the API, wherever it was ambiguous I left it nullable, therefore as it was. There's one or 2 behaviours under test affected, fairly edge case ones that are just as well off IMO. See the diff on the tests.

Thanks for the great lib!

darwingr commented 3 years ago

This PR is really appreciated!

I think that a few nulls are not necessary. It would be great, if you improved them.

However I would merge your PR even without them (and try to fix them myself later)

Thanks a lot!

Glad to hear you're glad. I agree with you about the unnecessary nullables. Some of the ones you specifically pointed to I actually thought I could make work as non-nullable but when playing with the migration tool the changes propagated to affecting either the API, which I was not prepared to change, or required implementation changes to a degree that I figured you should have a say first.

As an initial pre-release version this seems to me like a nice limitation in scope despite the number of line changes. If you want to do few pre-releases or just one, it's totally your call.

I'll be able to take look at what you so conveniently highlighted above over the weekend. If you're wanting to try the migration tool yourself instead of just the analyzer/IDE, I would suggest checking out 9b4def1, make and move to a new branch, then run dart migrate. I don't think it works right once it's been migrated and that way you can compare with the more conservative changes that I made on master.'

Cheers!