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.

Sign up for GitHub

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

Jump to bottom

Migration to null safety #11

Merged
merged 5 commits into from Apr 20, 2021
Merged

Migration to null safety #11

merged 5 commits into from Apr 20, 2021

Conversation

afermin
Copy link
Contributor

@afermin afermin commented Apr 20, 2021 •
edited

Reopened:

Hi there,

I just migrated to null safety the plugin regarding this #10

I hope you can take a look and left some comments if needed. @maxnovak @longhorn499

Best.

Description

Motivation and Context

It's required to be able to migrate to dart null safety.

10

Type of Change:

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Maintenance or Refactor

Checklist

  • I added tests to cover my changes
  • I rebased changes with master so that they can be merged easily
  • I have updated the documentation accordingly (if applicable)

@afermin afermin mentioned this pull request Apr 20, 2021
Closed
@kevin49999 kevin49999 self-requested a review April 20, 2021 19:20
@afermin
Merge branch 'main' into migration_null_safety
0900d69 
# Conflicts:
#	lib/optimizely_plugin.dart
#	pubspec.yaml
maxnovak
maxnovak reviewed Apr 20, 2021
View reviewed changes
Copy link
Contributor

@maxnovak maxnovak left a comment

Choose a reason for hiding this comment

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

Could you please undo the format changes, which are unrelated to your PR?
e.g. the double to single quotes & tab changes

@afermin
Copy link
Contributor Author

afermin commented Apr 20, 2021

Could you please undo the format changes, which are unrelated to your PR?
e.g. the double to single quotes & tab changes

Done

@afermin
Copy link
Contributor Author

afermin commented Apr 20, 2021

Looks like the flutter version should updated in the CI

kevin49999
kevin49999 reviewed Apr 20, 2021
View reviewed changes
example/lib/main.dart Outdated Show resolved Hide resolved
kevin49999
kevin49999 reviewed Apr 20, 2021
View reviewed changes
example/lib/main.dart Outdated Show resolved Hide resolved
kevin49999
kevin49999 reviewed Apr 20, 2021
View reviewed changes
example/lib/main.dart Outdated Show resolved Hide resolved
@kevin49999
Copy link
Contributor

kevin49999 commented Apr 20, 2021 •
edited

Looks like the flutter version should updated in the CI

Yep, I think updating it to 2.0.4 (or any newer version past the latest major release) should do it there. You can update that in .github/workflows/ci.yaml and if that passes locally I can merge this in and do the release. Not sure if updating that on this branch will cause the action here to run with the higher version (or it will just run w/ the action settings on main) 🤔 but I can pull it locally if not and merge / follow-up and fix the action

@afermin
Copy link
Contributor Author

afermin commented Apr 20, 2021

Looks like the flutter version should updated in the CI

Yep, I think updating it to 2.0.4 (or any newer version past the latest major release) should do it there. You can update that in .github/workflows/ci.yaml and if that passes locally I can merge this in and do the release. Not sure if updating that on this branch will cause the action here to run with the higher version (or it will just run w/ the action settings on main) 🤔 but I can pull it locally if not and merge / follow-up and fix the action

Thanks for that. Already changed it and it worked

@kevin49999 kevin49999 self-requested a review April 20, 2021 21:07
kevin49999
kevin49999 approved these changes Apr 20, 2021
View reviewed changes
Copy link
Contributor

@kevin49999 kevin49999 left a comment

Choose a reason for hiding this comment

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

lgtm! 🌠 Thanks for contributing

@kevin49999 kevin49999 merged commit 46b2ed4 into policygenius:main Apr 20, 2021
@afermin
Copy link
Contributor Author

afermin commented Apr 20, 2021

lgtm! 🌠 Thanks for contributing

My pleasure. Thank you for letting me do that!

@kevin49999
Copy link
Contributor

kevin49999 commented Apr 20, 2021 •
edited

lgtm! 🌠 Thanks for contributing

My pleasure. Thank you for letting me do that!

Of course, thanks again. It's published. There are notes in the release on GitHub, but I will follow-up with an update in the CHANGELOG as well as fix some warnings I found when running dart pub publish --dry-run

@kevin49999 kevin49999 mentioned this pull request Apr 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxnovak maxnovak maxnovak left review comments

@kevin49999 kevin49999 kevin49999 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@afermin @kevin49999 @maxnovak