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

Forced null safety #287

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Forced null safety #287

merged 8 commits into from
Apr 23, 2021

Conversation

thomassth
Copy link
Contributor

After updating dependencies and running dart migrate, and with a few manual patches, this is the kind-of functional null safety enabled device_calendar.

Tested and working functions (because I use them):

  • retrieveCalendars() retrieves calendar on Android
  • Event event(calendarId_String) can be set correctly (although I still find it confusing to use a number as a string)
  • createOrUpdateEvent() added calendar event to the selected calendar

If anyone is interested please test it with other functions and see if they run, and better yet, help fixing other bugs.

To use this instead of the published package:

  1. Save it to somewhere convenient, like next to your app package:

Parent Folder
YourApp
device_calendar_null

  1. add this to your pubspec.yaml

dependencies:

device_calendar:

path: ../device_calendar/
  1. Run flutter pub get

@thomassth thomassth mentioned this pull request Mar 23, 2021
@andzejsw
Copy link
Contributor

andzejsw commented Mar 23, 2021

Main difference from my PR #286 as far as I see is that example uses old flutter SDK. Tested both PR for my currently required project and both are working as expected. I had problems for this PR for adding new calendar in example.

@thomassth
Copy link
Contributor Author

I have manually merged #286 to this pull request, and fixed up the example a bit. Some logic may need to be refined but overall this is sound null safety, example app compiled successfully in iOS and android, and passed auto checks.

TODO:

  • iOS app not requesting for calendar permission by itself, has to be called upon; android's calendar prompt pop up as expected

andzejsw
andzejsw previously approved these changes Apr 6, 2021
Took me way too long to find this line.
@andzejsw
Copy link
Contributor

Should I merge this PR @thomassth?

Copy link
Contributor

@nickrandolph nickrandolph left a comment

Choose a reason for hiding this comment

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

Hey, I assume this will be a breaking change for devs using this package? If so, we'll need to increment the major version number.
Also, can you make sure the readme is updated to reflect changes in usage and the Changelog has details of what's changed.

@andzejsw
Copy link
Contributor

Hey, I assume this will be a breaking change for devs using this package? If so, we'll need to increment the major version number.
Also, can you make sure the readme is updated to reflect changes in usage and the Changelog has details of what's changed.

Sure. No problem. I will take a look. An yes, this is breaking change.

@nickrandolph
Copy link
Contributor

Make sure you update pubspec with new version number (as breaking change, make sure you increment major version number). The changelog should include the version number.
The CI/CD pipeline should handle publishing dev builds of the package once this has been merged.

@andzejsw
Copy link
Contributor

Ok. Thanks for help. I will make sure that everything is correct and will add required things.

@andzejsw
Copy link
Contributor

One thing to add. I see that @thomassth changed to use LTS NDK version. Shouldn't we use latest stable?

@andzejsw
Copy link
Contributor

andzejsw commented Apr 22, 2021

I will keep now NDK version as it is currently in this PR, cause pipelines works.

@thomassth
Copy link
Contributor Author

Should we publish this as a prerelease? (4.0.0-dev.1)
Or do you guys think it's stable enough
https://dart.dev/tools/pub/publishing#publishing-prereleases

@andzejsw
Copy link
Contributor

I was using my PR in my production app and today tested a bit this PR. In my opinion it is safe to publish 4.0.0 version. In any case, we can publish later some bugfix version if it is needed. But haven't seen any bug related to this PR.
By the way, I like your latest "Readme" changes.
If there is need, I can take a look on this PR tomorrow.

@nickrandolph
Copy link
Contributor

With the versioning - update version in pubspec without any prerelease flag. This is done automatically by the CI/CD pipeline. When you PR to develop I think we push a pre release.
I would recommend for 4.0 that we pick up some additional changes to fix the issues with timezones

@thomassth
Copy link
Contributor Author

In that case I think we are ready to merge to dev branch

@nickrandolph nickrandolph merged commit 3b8318a into builttoroam:develop Apr 23, 2021
@andzejsw
Copy link
Contributor

andzejsw commented Apr 23, 2021

@nickrandolph should we implement "nickr/timezones" branch changes into development branch or there logic wasn't fully functioning?

@andzejsw andzejsw mentioned this pull request Apr 23, 2021
@nickrandolph
Copy link
Contributor

That branch should be the next section of work - it was a while ago but I think I got most of the way through the changes to Android in order to work with the new dart entities. I'd suggest taking that branch for a run and seeing what's left to do.

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.

3 participants