Code review comment for lp:~ubuntu-weather-dev/ubuntu-weather-app/new-weather-api

Revision history for this message
Andrew Hayzen (ahayzen) wrote :

1) Done
2) Done bumped the version and put the relevant stuff in the sections
3) I've commented out the 'dead code' where it uses the location code for TWC, let me know if you want me to remove

Also, I've spotted one todo on line 44 of the diff
+ var partData = dataObj["metric"] || dataObj["imperial"] || dataObj; // TODO: be more intelligent?

Do you think this is OK? or should it be more like?
if (there is metric) { use metric } else if (there is imperial) { use imperial } else { fallback }

Futhermore the lines below have things like calcFahrenheit() which convert from metric->imperial, so it assumes the data is metric, maybe we should remove the imperial option... so just

+ var partData = dataObj["metric"] || dataObj;

I think i've talked myself into changing it to that :')

review: Needs Information

« Back to merge proposal