Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

nit: Add a short comment explain path dependencies #2511

Closed
wants to merge 329 commits into from
Closed

nit: Add a short comment explain path dependencies #2511

wants to merge 329 commits into from

Conversation

jonasfj
Copy link
Member

@jonasfj jonasfj commented Feb 10, 2020

Adds a comment # ^x.y.z in your application where examples have path dependencies.

This was motivated by dart-lang/pub#1850 (comment) where a user is running into problems by copying the dependency from an example/pubspec.yaml.

Maybe we shouldn't make it all one-line... and maybe we could phrase the comment better.
Or maybe we shouldn't even do this -- feel free to close this if you think this is too obvious :)

@jonasfj jonasfj requested a review from amirh February 10, 2020 12:43
amirh
amirh reviewed Feb 10, 2020
View reviewed changes
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

Thanks!
Left 2 nits.
You may also want to update the plugin template here: https://github.com/flutter/flutter/blob/5f4658973be1ad10a59a0a278b6cdf41b10fe7f6/packages/flutter_tools/templates/app/pubspec.yaml.tmpl#L43

packages/android_alarm_manager/example/pubspec.yaml Outdated
@@ -4,8 +4,7 @@ description: Demonstrates how to use the android_alarm_manager plugin.
dependencies:
flutter:
sdk: flutter
android_alarm_manager:
path: ../
android_alarm_manager: {path: ../} # ^x.y.z in your application
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't care if this is one line or 2 lines, though I don't want these to go back-and-forth between the 2 styles.
This is the kind of things we should write in a style guide, the closest thing to a style guide I found are the examples here: https://dart.dev/tools/pub/dependencies (which are using 2 lines). If we believe this style is preferred we should probably update that page as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I only used the one-liner style because it put the comment on the same line.

packages/android_alarm_manager/example/pubspec.yaml Outdated
@@ -4,8 +4,7 @@ description: Demonstrates how to use the android_alarm_manager plugin.
dependencies:
flutter:
sdk: flutter
android_alarm_manager:
path: ../
android_alarm_manager: {path: ../} # ^x.y.z in your application
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd make the comment more verbose so it's easier to understand why it's a path dependency here, maybe something like:

The example app is bundled with the plugin so we use a path dependency on the parent directory to use the current plugin's version. When depending on this package from a real application you should use : plugin_name: ^x.y.z
See https://dart.dev/tools/pub/dependencies#version-constraints for more information.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, that's better than using one-line :)

@jonasfj jonasfj force-pushed the add-path-import-comment branch from d3f50fa to 03ee13f Compare February 18, 2020 16:48
@jonasfj jonasfj mentioned this pull request Feb 18, 2020
Merged
amirh
amirh approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

Version checker is complaining as we did not bump package versions, I'm ok with landing this without publishing assuming people mostly check the examples in GitHub and not from the pub tarball, though it's an interesting thought to run by @jonasfj 😄

@jonasfj jonasfj requested a review from amirh February 18, 2020 18:05
@jonasfj
Copy link
Member Author

jonasfj commented Feb 18, 2020

@amirh, I just updated the PR to include publish_to: 'none' for all example plugins.

They aren't meant to be published I assume, and the template already has these:
https://github.com/flutter/flutter/blob/5f4658973be1ad10a59a0a278b6cdf41b10fe7f6/packages/flutter_tools/templates/app/pubspec.yaml.tmpl#L4

@jonasfj jonasfj force-pushed the add-path-import-comment branch 2 times, most recently from cfd78db to be20c9f Compare March 17, 2020 09:16
@jonasfj
Copy link
Member Author

jonasfj commented Mar 17, 2020

@amirh, I've updated this and flutter/flutter#50978, if you're still happy with this let's ship it :)

@jonasfj
Copy link
Member Author

jonasfj commented Mar 17, 2020

Please merge if you're happy with this.

@ened
Copy link
Contributor

ened commented Apr 17, 2020

hi @jonasfj could you please rebase & take a look at the CI errors?

@jonasfj jonasfj force-pushed the add-path-import-comment branch from be20c9f to 21c6066 Compare April 17, 2020 10:16
@jonasfj
Copy link
Member Author

jonasfj commented Apr 17, 2020

Not sure why CI would fail.

ditman and others added 15 commits September 30, 2020 13:37
@ditman
[url_launcher_web] Move third_party under src. (flutter#3080)
dac9bc0
@amirh
[google_maps_flutter] Out of developers preview, bump to 1.0.0 (flutt…
156ec5e 
…er#3091)
@bparrishMines
[webview_flutter] Add new entrypoint that uses hybrid composition on …
6d8b156 
…Android (flutter#3067)
[google_maps_flutter] Fix headline in the readme (flutter#3100)
9b04cf9
Remove io.flutter.embedded_views_preview from README
18771ff
@hamdikahloun
[android_intent] Android Code Inspection and Clean up (flutter#3043)
cc9bf02
@hamdikahloun
[connectivity] Android Code Inspection and Clean up (flutter#3051)
c696333
@gaaclarke
in_app_purchase: started supported null as a parameter for the sandbo…
3a5d9ed 
…x arguement (flutter#3110)
[webview_flutter] add public documentation. (flutter#3114)
a3f7e69
@ditman
[multiple] Opt-out tests of null-safety (flutter#3113)
755a73c
@ditman
[google_maps_flutter_web] Fix convert.dart issues (flutter#3093)
2077363 
* Convert initial Polyline/Polygon points correctly.
* Convert Flutter Color to CSS correctly.

Fixes flutter/flutter#65152
Fixes flutter/flutter#67032
@dennis-tra
[in_app_purchase] Fix finishing purchases upon payment cancellation (f…
05b1010 
…lutter#3106)
@hamdikahloun
Android Code Inspection and Clean up (flutter#3117)
8c9908e
@hamdikahloun
[in_app_purchase] Android Code Inspection and Clean up (flutter#3120)
c8e3aa1 
* Android Code Inspection and Clean up

* Format
@hamdikahloun
[video_player] Fix SSLProtocolException for low API version (flutter#…
53e8f47 
…3122)
@jonasfj
Copy link
Member Author

jonasfj commented Feb 12, 2021

Okay, I'm going to close this... Not sure what mess I just made of the history 🙈

@jonasfj jonasfj closed this Feb 12, 2021
@jonasfj jonasfj deleted the add-path-import-comment branch February 12, 2021 14:58
@google-cla
Copy link

google-cla bot commented Feb 12, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Feb 12, 2021
@stuartmorgan-g
Copy link
Contributor

@jonasfj If you want to do a clean version I can review it ASAP so it doesn't bitrot.

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

@amirh amirh Awaiting requested review from amirh

@bparrishMines bparrishMines Awaiting requested review from bparrishMines

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@ditman ditman Awaiting requested review from ditman

@dnfield dnfield Awaiting requested review from dnfield

@gaaclarke gaaclarke Awaiting requested review from gaaclarke

@LHLL LHLL Awaiting requested review from LHLL

@mvanbeusekom mvanbeusekom Awaiting requested review from mvanbeusekom

Assignees
No one assigned
Labels
cla: no p: android_alarm_manager p: android_intent p: battery Edits files for a battery plugin p: camera p: connectivity p: cross_file p: device_info p: espresso p: file_selector p: flutter_plugin_android_lifecycle p: google_maps_flutter p: google_sign_in p: image_picker p: in_app_purchase p: integration_test p: ios_platform_images p: local_auth p: package_info p: path_provider p: plugin_platform_interface p: quick_actions p: sensors p: share p: shared_preferences p: url_launcher p: video_player p: webview_flutter Edits files for a webview_flutter plugin p: wifi_info_flutter platform-android platform-ios platform-linux platform-macos platform-web platform-windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.