[custom-devices] general improvements, add custom-devices subcommand, better error handling by ardera · Pull Request #82043 · flutter/flutter

ardera · 2021-05-07T17:42:58Z

generally improves the custom devices feature
adds flutter subcommand for custom-devices management:
- flutter custom-devices list lists the configured custom devices
  - can be used to check for errors
  - will also output the path of the config
- flutter custom-devices reset
  - resets the configured custom devices config to default & creates a backup
- flutter custom-devices add
  - either supply a json-formatted string as an argument to the --json parameter
  - or configure using an interactive, guided device setup
  - that guided setup will by default setup a ssh device, i.e. it won't ask for the values of every custom device config option, but rather for things like the ssh username & hostname and generally provide some good defaults for ssh devices
- flutter custom-devices delete
  - deletes a custom-devices file based on the device id
better error checking when loading the config & better error messages
if anything goes wrong while loading a CustomDeviceConfig, a JsonRevivalException will be thrown with an error message describing the error (like Expected 'pingSuccessRegex' to be null or string-ified regex.)
that error will also be catched and expanded a bit by CustomDevicesConfig.devices so the outputted error message may look like this: Could not load custom device from config index 0: Expected 'pingSuccessRegex' to be null or string-ified regex.

Pre-launch Checklist

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

flutter-dashboard · 2021-05-27T17:45:21Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

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

HidenoriMatsubayashi · 2021-05-30T13:48:14Z

Great work! I have one feedback about custom-devices.schema.json. There may be some reason, but I thought enabled is more intuitive than disabled ｗhen I first saw it.

          "disabled": {
            "description": ~,
            "type": "boolean"
            "type": "boolean",
            "default": false
          },

ardera · 2021-05-30T14:01:36Z

Great work! I have one feedback about custom-devices.schema.json. There may be some reason, but I thought enabled is more intuitive than disabled ｗhen I first saw it.

Thanks! Yeah, the style guide also mentions this, I chose to ignore it because I thought it'd be weird for a field to default to "true" if omitted, but you're probably right I should've called it enabled instead. Also leads to some awkward double negatives in the code

I'll change it 👍

ardera · 2021-06-04T00:50:48Z

this should be ready to be reviewed now @jonahwilliams (just fyi)

jonahwilliams · 2021-06-04T01:29:38Z

Awesome, will take a look at this tomorrow!

jonahwilliams

Still looking through this one, just some initial feedback