Skip to content
This repository has been archived by the owner on Mar 18, 2021. It is now read-only.

Jc/responseencoder #22

Merged
merged 10 commits into from
Nov 30, 2015
Merged

Jc/responseencoder #22

merged 10 commits into from
Nov 30, 2015

Conversation

itsjoeconway
Copy link
Contributor

Applications can now be stopped (this is good for tests), and they can wait for stop to finish.

Updated Application to use new Dart 1.13 API for starting HTTPS sever.

Privatized HttpController's response type/encoder and added a public-only method for setting both at the same time.

Code cleanup, test updates.

reflectedThat.type.declarations.values.where((dm) => dm is VariableMirror).forEach((VariableMirror vm) {
reflectedThis.setField(vm.simpleName, reflectedThat.getField(vm.simpleName).reflectee);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty cool, but it seems kinda dangerous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so? I realized it was more dangerous when I was setting each property individually, added a new property, and it wasn't showing up on the other end of the pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried about the implicit assumption that all of the properties are copied on assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok nvm I looked at how its being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, that brings up a good point: there really isn't a need to copy this information anymore. The purpose was to copy it for each isolate, but that is no longer necessary. removing

@itsjoeconway
Copy link
Contributor Author

OK, so I went ahead and added inquirer directly to this repo since that was causing issues as well

@anachlas
Copy link
Contributor

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants