Skip to content

Conversation

@devoncarew
Copy link
Contributor

  • return an error code for hot reload instead of an exception result

In IDEs, we can show:

Screen Shot 2019-04-18 at 9 12 13 AM

instead of:

Screen Shot 2019-04-14 at 12 12 25 AM

I'm going to let this build through in the bots - I'm having trouble running the tests locally.

@devoncarew devoncarew requested review from grouma and jakemac53 April 18, 2019 18:24
@devoncarew devoncarew merged commit 3a75266 into master Apr 18, 2019
@kevmoo kevmoo deleted the hot_reload_message branch April 18, 2019 19:42
throw ArgumentError.value(
fullRestart, 'fullRestart', 'We do not support hot reload yet.');
return {
'code': 1,
Copy link
Member

Choose a reason for hiding this comment

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

The code -32601 is standardized in the JSON RPC spec to mean "Method not found". Should we use that here?

Copy link
Member

Choose a reason for hiding this comment

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

The method is found just not implemented so I don't think that makes sense here.

Copy link
Member

Choose a reason for hiding this comment

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

https://www.jsonrpc.org/specification#error_object

The meaning is "The method does not exist / is not available." - IMO "not available" applies here.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh. Makes more sense to me then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not spec'd in the daemon protocol what non-zero codes look like. Clients are written to use 0 to mean success, and non-zero failure. The only other daemon protocol impl (in flutter_tools) uses 1 for a restart failed code.

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