Skip to content

[pigeon] 0.2.0 Made null-safe generation the default, updated documentation #315

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
Merged
gaaclarke merged 7 commits into from
Mar 29, 2021
Merged

[pigeon] 0.2.0 Made null-safe generation the default, updated documentation #315

gaaclarke merged 7 commits into from
Mar 29, 2021

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Mar 19, 2021

Most of the work for 0.2.0 happened in an external contributors commit: #300

Pre-launch Checklist

  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke
Copy link
Member Author

gaaclarke commented Mar 19, 2021

cc @Goddchen

@Goddchen
Copy link
Contributor

Goddchen commented Mar 20, 2021

@gaaclarke I have the fixes for the tests ready. But I don't know how I can get them in here. I tried checking out this repo/branch, committed them locally and tried to push them. But obviously I don't have write permission in here...

@Goddchen
Copy link
Contributor

Goddchen commented Mar 20, 2021

Hopefully this will to the trick: gaaclarke#1

@gaaclarke
Copy link
Member Author

gaaclarke commented Mar 22, 2021

@Goddchen Thanks, that was kind of you. There is something weird going on with our CI. They are running locally fine and I can't retry. I'll keep your PR handy while I try to kick the bots.

@Goddchen
Copy link
Contributor

Goddchen commented Mar 22, 2021

What's going on with the checks? They seem to be hanging again?

@gaaclarke
Copy link
Member Author

gaaclarke commented Mar 22, 2021

@Goddchen yea, I'm pinging a few people from my side to see if I can get it resolved.

@gaaclarke gaaclarke requested a review from xster March 23, 2021 16:55
@mitchross
Copy link

mitchross commented Mar 24, 2021 •
edited by gaaclarke
Loading

Seeing a crash running this.

pubspec

dev_dependencies:
  flutter_test:
    sdk: flutter
  pigeon: 
    git:
      url: https://github.com/gaaclarke/packages.git
      path: packages/pigeon
      ref: pigeon-2-0-0`

flutter pub run pigeon --input pigeons/messages.dart --dart_null_safety
Unhandled exception:
Null check operator used on a null value
#0      generateJava (package:pigeon/java_generator.dart:248:49)
#1      Pigeon.run.<anonymous closure> (package:pigeon/pigeon_lib.dart:486:34)
#2      Pigeon._runGenerator (package:pigeon/pigeon_lib.dart:357:9)
#3      Pigeon.run (package:pigeon/pigeon_lib.dart:484:15)
<asynchronous suspension>
#4      main (file:///C:/Users/xxxx/AppData/Local/Temp/flutter_pigeon.94186523/_pigeon_temp_.dart:8:17)    
<asynchronous suspension>

image

@gaaclarke
Copy link
Member Author

gaaclarke commented Mar 24, 2021

@mitchross Weird, it's working in the tests and when I try it out. Any idea what might be different for you?

$ git status
On branch pigeon-2-0-0
Your branch is up to date with 'origin/pigeon-2-0-0'.

$ pub run pigeon --input pigeons/message.dart --dart_null_safety --java_out foo.java --dart_out /dev/null

What does your messages.dart file look like?

@alr2413
Copy link

alr2413 commented Mar 24, 2021

@gaaclarke,
The same happens for me.
You just need to clone the flutter vlc player repository https://github.com/solid-software/flutter_vlc_player.git
and then inside the flutter_vlc_player folder run pigeon command.

@mitchross
Copy link

mitchross commented Mar 24, 2021

@gaaclarke,
The same happens for me.
You just need to clone the flutter vlc player repository https://github.com/solid-software/flutter_vlc_player.git
and then inside the flutter_vlc_player folder run pigeon command.

Also swap out the pubspec

dev_dependencies:
flutter_test:
sdk: flutter
pigeon:
git:
url: https://github.com/gaaclarke/packages.git
path: packages/pigeon
ref: pigeon-2-0-0`

@gaaclarke
Copy link
Member Author

gaaclarke commented Mar 24, 2021

@mitchross @alr2413 Thanks, should be fixed now.

xster
xster approved these changes Mar 24, 2021
View reviewed changes
packages/pigeon/CHANGELOG.md Outdated
Copy link
Member

@xster xster Mar 24, 2021

Choose a reason for hiding this comment

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

add bug reference

Copy link
Member Author

@gaaclarke gaaclarke Mar 29, 2021

Choose a reason for hiding this comment

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

done

packages/pigeon/README.md Outdated
Copy link
Member

@xster xster Mar 24, 2021

Choose a reason for hiding this comment

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

Since it's a breaking change, add a banner at the top of this page too to describe where the break is

packages/pigeon/README.md Outdated
Copy link
Member

@xster xster Mar 24, 2021

Choose a reason for hiding this comment

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

We should caveat this a bit more clearly. If we're highlighting null safety more prominently, we should be clear there are 2 layers. There's the general null safety concept and there we're not doing too much yet. Things can't be defined as non-null and the nullable types aren't specifically annotated as so in kotlin and swift. There's also the Dart 2.12 null safety mechanism. We don't support non-null neither but we properly tag everything as nullable now.

Copy link
Member Author

@gaaclarke gaaclarke Mar 29, 2021

Choose a reason for hiding this comment

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

done

This was referenced Mar 26, 2021
Closed
Merged
Goddchen referenced this pull request Mar 26, 2021
@Goddchen
[pigeon] migrate to null-safety (#300)
6a92ec5 
[pigeon] migrated to null-safety ( v0.2.0)

Co-authored-by: Aaron Clarke <gaaclarke>
@Goddchen
Copy link
Contributor

Goddchen commented Mar 26, 2021

Failing after 1441m These checks are really screwed sometimes... I wish there was a simple "retry" button...

@gaaclarke gaaclarke merged commit 1378c44 into flutter:master Mar 29, 2021
@gaaclarke
Copy link
Member Author

gaaclarke commented Mar 29, 2021

Okay, published. Thanks for the patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xster xster xster 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.

5 participants

@gaaclarke @Goddchen @mitchross @alr2413 @xster