Updated obsolete URL by Dmitry-Borodin · Pull Request #52158 · flutter/flutter

Dmitry-Borodin · 2020-03-07T05:16:25Z

Description

Replaced obsolete URL to actual one

Related Issues

Tests

I added no tests:

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
[] The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
Actually, this^^ reports thousands of errors. But they are not related to my changes.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

[ x] No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

fluttergithubbot · 2020-03-07T05:16:27Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

googlebot · 2020-03-07T05:16:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Dmitry-Borodin · 2020-03-07T05:36:58Z

@googlebot I signed it!

googlebot · 2020-03-07T05:37:02Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

jacob314 · 2020-03-12T17:59:11Z

that link still loses the #android-setup by the time you select your platform.

it won't be https://flutter.dev/docs/get-started/install anymore, I think it's still better then https://flutter.dev/setup/ that is redirect only

Would https://flutter.dev/docs/get-started/install without the anchor be better (since it doesn't imply any android info), plus create an issue to switch on https://flutter.dev/docs/get-started/install/windows, https://flutter.dev/docs/get-started/install/macos etc based on the current platform?

Or you can make that platform switch happen in this review if you want, @Dmitry-Borodin

Will check tomorrow how can easy can I get the platform in dart.

@jmagman updated PR, made urls platform dependent, please take a look
I tested some random messages on my computer, but don't see a nice possibility how to add autotests to the project.
The code is straightforward and can run on any platform, so test will just copy the code it should test

Dmitry-Borodin · 2020-03-21T11:38:51Z

I see CI error "flutter build is currently broken", but I don't see if I should do something about it. Looks like it's not related to my changes.

jmagman · 2020-03-23T18:25:21Z

Try:

import 'globals.dart' as globals;

String get _androidSdkInstallUrl { if (globals.platform.isMacOS) { return 'https://flutter.dev/docs/get-started/install/macos#android-setup'; } ...

That I cannot compile on my machine

Unable to spawn isolate: lib/src/base/user_messages.dart:6:8: Error: Error when reading 'lib/src/base/globals.dart': No such file or directory import 'globals.dart' as globals; ^

Why dart:io is forbidden? Isn't it a standard library of the language?

That I cannot compile on my machine

I was just indicating how this is done everywhere else (so you can mock out the platform you're on in tests). grep around for isMacOS and you'll see how it's done.
You need to adjust the import to point to the relative path:

import '../globals.dart' as globals;

Why dart:io is forbidden? Isn't it a standard library of the language?

You would import lib/src/base/io.dart if you need something in there, see #7390 and #7385 re:testability.
But in your case the mockable platform logic is in globals.dart so just import that.

Thank you very much for clarification. Done.

Dmitry-Borodin · 2020-03-24T12:33:01Z

^^ rebased on upstream master

Dmitry-Borodin · 2020-03-24T20:45:49Z

Checked tests CI writing to logs - like devices_test.dart and ios_device_start_prebuilt_test.dart - those looks unrelated to my changes and are passing on my machine.

fixes flutter#51815

jmagman · 2020-03-26T01:06:24Z

It is related.

  Unsupported operation: context.get<Platform> is not supported in test methods. Use Testbed or testUsingContext if accessing Zone injected values.
  test\src\common.dart 190:5                                    NoContext.get
  package:flutter_tools/src/globals.dart 69:34                  platform
  package:flutter_tools/src/base/user_messages.dart 298:17      UserMessages._androidSdkInstallUrl
  package:flutter_tools/src/base/user_messages.dart 115:18      UserMessages.androidSdkBuildToolsOutdated
  test\general.shard\android\android_workflow_test.dart 229:46  main.<fn>
  dart:async                                                    runZoned
  test\src\common.dart 167:14                                   testWithoutContext.<fn>

I took the liberty of pushing a new commit to your fork that injects the platform into UserMessages for testability, and I wrote unit tests for the change.

jmagman · 2020-03-26T01:09:23Z

@zanderso Would you mind reviewing since I wrote part of it? None of these strings are overridden in g3.

zanderso

lgtm w/ nits

zanderso · 2020-03-26T03:00:33Z

+            _androidSdk.sdkManagerPath,
+            kAndroidSdkMinVersion,
+            kAndroidSdkBuildToolsMinVersion.toString(),
+            _platform


nit: need a trailing comma.

zanderso · 2020-03-26T03:04:24Z

+
+  String _androidSdkInstallUrl(Platform platform) {
+    if (platform.isMacOS) {
+      return 'https://flutter.dev/docs/get-started/install/macos#android-setup';


Maybe pull 'https://flutter.dev/docs/get-started/install' and 'android-setup' out into locals to avoid repeating them.

Done, though I think it's harder to read.
(The more correct thing would be to construct a Uri but that seems over-engineered for this one.)

jmagman · 2020-03-26T21:34:40Z

Thank you for your contribution, @Dmitry-Borodin!

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 7, 2020

googlebot added the cla: no label Mar 7, 2020

googlebot added cla: yes and removed cla: no labels Mar 7, 2020

zanderso requested a review from jacob314 March 12, 2020 17:53

jacob314 reviewed Mar 12, 2020

View reviewed changes

zanderso added the waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds label Mar 19, 2020

jmagman requested changes Mar 23, 2020

View reviewed changes

Dmitry-Borodin force-pushed the url-fix branch from 686401f to 5d29bc1 Compare March 24, 2020 12:31

Dmitry-Borodin and others added 6 commits March 25, 2020 17:26

updated obsolete URL

efe7a6f

fixes flutter#51815

made android sdk url for error messages platform dependent

a8166c0

refactored function no args => property

901be8d

Fix indentation to two spaces.

606c267

switching dart:io to global.dart

298327d

Inject platform into user messages, add unit test

499e313

jmagman force-pushed the url-fix branch from 5d29bc1 to 499e313 Compare March 26, 2020 01:05

Add missing endline, add bogus platform

fae3cea

zanderso approved these changes Mar 26, 2020

View reviewed changes

jmagman added 2 commits March 26, 2020 11:07

Review edits

7106606

Anchor -> fragment

2baabef

jmagman approved these changes Mar 26, 2020

View reviewed changes

jmagman added waiting for tree to go green and removed waiting for response The Flutter team cannot make further progress on this issue until the original reporter responds waiting for tree to go green labels Mar 26, 2020

jmagman merged commit 5a3e7e4 into flutter:master Mar 26, 2020

Dmitry-Borodin deleted the url-fix branch March 26, 2020 23:41

github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Conversation

Dmitry-Borodin commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Mar 7, 2020

Uh oh!

googlebot commented Mar 7, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Dmitry-Borodin commented Mar 7, 2020

Uh oh!

googlebot commented Mar 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitry-Borodin commented Mar 21, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dmitry-Borodin commented Mar 24, 2020

Uh oh!

Dmitry-Borodin commented Mar 24, 2020

Uh oh!

jmagman commented Mar 26, 2020

Uh oh!

jmagman commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Mar 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dmitry-Borodin commented Mar 7, 2020 •

edited

Loading

jmagman Mar 23, 2020 •

edited

Loading

jmagman commented Mar 26, 2020 •

edited

Loading