Annotations working with Go core.

Annotations working with Go core.

Annotations send the correct Tag to core.  Additionally, upon fixing this, it was discovered that annotations were not being correctly applied to their objects given the new format of the delta stream.  This was fixed, as well as a type conversion error in x/y coordinate annotations.

https://code.launchpad.net/~makyo/juju-gui/entity-name-1161852/+merge/156670

(do not edit description out of merge proposal)

[matthew.scott, 2013-04-02 19:52:20]

Please take a look.

[benji, 2013-04-02 20:38:11]

LGTM, I only had a couple of small (I hope) requests.

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js#newcode27
app/config-debug.js:27: apiBackend: 'go', // Value can be 'python' or 'go'.
I don't think we want to change the default config just yet.

https://codereview.appspot.com/8280043/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8280043/diff/1/app/models/models.js#newcode547
app/models/models.js:547: **/
Yay comments!

Boo, they are now written in at least four styles in different parts of the code
base.

Maybe the yuidoc linter should be extended to keep us on the rails.

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js
File app/store/env/go.js (right):

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode143
app/store/env/go.js:143: id: entityInfo.Tag.replace(/^[^\-]+-/, ''),
Future generations will thank us if this regex were in its own helper function
and had tests that demonstrated the expected inputs and outputs of the string
manipulation.

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode516
app/store/env/go.js:516: * @param {String{ type The type of entity that is being
annotated
that second "{" should be a "}".

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode544
app/store/env/go.js:544: * @param {String{ type The type of entity that is being
annotated
Here is another { that needs to be rotated 180 degrees.

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode597
app/store/env/go.js:597: * @return {Object} A dictionary of key,value pairs is
returned in the
The attack of the backward curly braces continues.

https://codereview.appspot.com/8280043/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/8280043/diff/1/app/views/utils.js#newcode668
app/views/utils.js:668: this['_' + name] = parseFloat(value);
parseFloat is really sloppy (as I'm sure you know), but I can't see any way it
will bite us here.  Can you?

[bac, 2013-04-02 20:38:57]

LGTM with a few requested changes.

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js#newcode23
app/config-debug.js:23: //socket_protocol: 'ws',
This is leftover from debugging, right?  This whole file should be reverted.

https://codereview.appspot.com/8280043/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8280043/diff/1/app/models/models.js#newcode555
app/models/models.js:555: change[2].type :
Please comment what change[2] is.  Looks magical.

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js
File app/store/env/go.js (right):

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode143
app/store/env/go.js:143: id: entityInfo.Tag.replace(/^[^\-]+-/, ''),
An English description of what the regex is doing would be nice.

[matthew.scott, 2013-04-02 23:18:04]

Thanks for the reviews.  I've moved to the following YUIdoc style comments, per
hatch:

/**
  comment body
*/

Also created a card to discuss this.

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js#newcode23
app/config-debug.js:23: //socket_protocol: 'ws',
On 2013/04/02 20:38:57, bac wrote:
> This is leftover from debugging, right?  This whole file should be reverted.

reverted

https://codereview.appspot.com/8280043/diff/1/app/config-debug.js#newcode27
app/config-debug.js:27: apiBackend: 'go', // Value can be 'python' or 'go'.
 2013/04/02 20:38:11, benji wrote:
> I don't think we want to change the default config just yet.

reverted

https://codereview.appspot.com/8280043/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8280043/diff/1/app/models/models.js#newcode547
app/models/models.js:547: **/
On 2013/04/02 20:38:11, benji wrote:
> Yay comments!
> 
> Boo, they are now written in at least four styles in different parts of the
code
> base.
> 
> Maybe the yuidoc linter should be extended to keep us on the rails.

Done.

https://codereview.appspot.com/8280043/diff/1/app/models/models.js#newcode555
app/models/models.js:555: change[2].type :
On 2013/04/02 20:38:57, bac wrote:
> Please comment what change[2] is.  Looks magical.

Done.

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js
File app/store/env/go.js (right):

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode143
app/store/env/go.js:143: id: entityInfo.Tag.replace(/^[^\-]+-/, ''),
On 2013/04/02 20:38:11, benji wrote:
> Future generations will thank us if this regex were in its own helper function
> and had tests that demonstrated the expected inputs and outputs of the string
> manipulation.

Done.

https://codereview.appspot.com/8280043/diff/1/app/store/env/go.js#newcode143
app/store/env/go.js:143: id: entityInfo.Tag.replace(/^[^\-]+-/, ''),
On 2013/04/02 20:38:57, bac wrote:
> An English description of what the regex is doing would be nice.

Done.

https://codereview.appspot.com/8280043/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/8280043/diff/1/app/views/utils.js#newcode668
app/views/utils.js:668: this['_' + name] = parseFloat(value);
On 2013/04/02 20:38:11, benji wrote:
> parseFloat is really sloppy (as I'm sure you know), but I can't see any way it
> will bite us here.  Can you?

I don't believe so, and we should have floats as dragging a service at a zoom
level other than an integer will cause these to be float values.

[matthew.scott, 2013-04-02 23:30:39]

*** Submitted:

Annotations working with Go core.

Annotations send the correct Tag to core.  Additionally, upon fixing this, it
was discovered that annotations were not being correctly applied to their
objects given the new format of the delta stream.  This was fixed, as well as a
type conversion error in x/y coordinate annotations.

R=benji, bac
CC=
https://codereview.appspot.com/8280043