Issue 189066: using new way to display stop markers
Patch Set 1 #
Total comments: 7
Patch Set 2 : updated #Patch Set 3 : fixed some bugs #
Total comments: 30
Patch Set 4 : updated #Patch Set 5 : updated #Patch Set 6 : updated #Patch Set 7 : updated #Patch Set 8 : updated #Patch Set 9 : update #Patch Set 10 : update #Patch Set 11 : updated #
Total comments: 12
Patch Set 12 : updated #
Total comments: 1
MessagesTotal messages: 12
1. using stop to display markers. 2. remove useless function in original code. 3. disable stop select function to keep upload smaller 4. bind dom using javascript instead of html global functions.
Sign in to reply to this message.
http://codereview.appspot.com/189066/diff/1/7 File schedule_editor.py (right): http://codereview.appspot.com/189066/diff/1/7#newcode140 schedule_editor.py:140: def handle_json_GET_boundboxstops(self, params): GET_stopswithinboundingbox http://codereview.appspot.com/189066/diff/1/7#newcode145 schedule_editor.py:145: """ Why copied? Is it possible to inherit the function from viewer and add some other helper function to support the information that you need? http://codereview.appspot.com/189066/diff/1/7#newcode152 schedule_editor.py:152: stops = schedule.GetStopsInBoundingBox(north=n, east=e, south=s, west=w, n=limit) line length <=80
Sign in to reply to this message.
updated http://codereview.appspot.com/189066/diff/1/7 File schedule_editor.py (right): http://codereview.appspot.com/189066/diff/1/7#newcode140 schedule_editor.py:140: def handle_json_GET_boundboxstops(self, params): On 2010/01/14 07:23:37, weiliu wrote: > GET_stopswithinboundingbox Done. http://codereview.appspot.com/189066/diff/1/7#newcode145 schedule_editor.py:145: """ On 2010/01/14 07:23:37, weiliu wrote: > Why copied? Is it possible to inherit the function from viewer and add some > other helper function to support the information that you need? can not be done. http://codereview.appspot.com/189066/diff/1/7#newcode152 schedule_editor.py:152: stops = schedule.GetStopsInBoundingBox(north=n, east=e, south=s, west=w, n=limit) On 2010/01/14 07:23:37, weiliu wrote: > line length <=80 Done.
Sign in to reply to this message.
http://codereview.appspot.com/189066/diff/1011/19 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/189066/diff/1011/19#newcode42 gtfsscheduleviewer/files/transit_editor.js:42: linePoints[linePoints.length] = new GLatLng(points[i][0], points[i][1]); Use .push() instead. http://codereview.appspot.com/189066/diff/1011/19#newcode122 gtfsscheduleviewer/files/transit_editor.js:122: gtfs.data.trips = {}; Add jsDoc comments for trips, stops and stopsOfShape. http://codereview.appspot.com/189066/diff/1011/19#newcode273 gtfsscheduleviewer/files/transit_editor.js:273: * @param {String} tripId Trip ID of stoptimes to be fetched Wrong parameter comment, please update it for "bounds". http://codereview.appspot.com/189066/diff/1011/19#newcode280 gtfsscheduleviewer/files/transit_editor.js:280: + "&e=" + bounds.getNorthEast().lng() Strange indentation. Use 4-char instead. http://codereview.appspot.com/189066/diff/1011/19#newcode860 gtfsscheduleviewer/files/transit_editor.js:860: if (Trip.current && Trip.current != data.trip) { In which cases Trip.current != this.curTrip_? http://codereview.appspot.com/189066/diff/1011/19#newcode946 gtfsscheduleviewer/files/transit_editor.js:946: * Reset Trip node to it's initial state This function seems not only to reset its node, but also the stops' status. Please correct the comment. http://codereview.appspot.com/189066/diff/1011/19#newcode946 gtfsscheduleviewer/files/transit_editor.js:946: * Reset Trip node to it's initial state Besides the node and stops, the trip has many other statuses, "reset" doesn't really reset all of them. So it still shouldn't has this name. Let's talk over phone about it. http://codereview.appspot.com/189066/diff/1011/19#newcode1042 gtfsscheduleviewer/files/transit_editor.js:1042: Remove these useless blank spaces. http://codereview.appspot.com/189066/diff/1011/19#newcode1061 gtfsscheduleviewer/files/transit_editor.js:1061: Trip.prototype.resetStop = function(isTripSelected) { "resetStop" implies it clears the trip's stop(s) to empty, but in fact it does not. It only changes the stops' status. Rename it to resetStopStatus. http://codereview.appspot.com/189066/diff/1011/19#newcode1200 gtfsscheduleviewer/files/transit_editor.js:1200: this.isTripSelected = false; Shouldn't be private? http://codereview.appspot.com/189066/diff/1011/19#newcode1212 gtfsscheduleviewer/files/transit_editor.js:1212: Stop.add = function(stopInfo) { Add comment. I like this static factory method, only a little comment on the name. According to the static factory method's naming pattern, it commonly is named as "create", "createXXX", "fromXXX" or "newXXX". I don't insist on the change though, please give it a consideration. http://codereview.appspot.com/189066/diff/1011/19#newcode1270 gtfsscheduleviewer/files/transit_editor.js:1270: * Create a labeled marker for the Stop object Add @param for arrivalTime http://codereview.appspot.com/189066/diff/1011/19#newcode1288 gtfsscheduleviewer/files/transit_editor.js:1288: * Set marker text Add @param for text. http://codereview.appspot.com/189066/diff/1011/19#newcode1329 gtfsscheduleviewer/files/transit_editor.js:1329: Stop.prototype.clear = function() { What is "clear" to clear? all statuses? or just the overlays? If it only clears the overlays, please rename it. Let's try to avoid this type of too general names. Talk about it over phone if you have questions. http://codereview.appspot.com/189066/diff/1011/19#newcode1495 gtfsscheduleviewer/files/transit_editor.js:1495: /* I just noticed that you make a small mistake in writing the jsDoc. The format of the comment is /** (two * in the first line, instead of one) * */ http://codereview.appspot.com/189066/diff/1011/19#newcode1534 gtfsscheduleviewer/files/transit_editor.js:1534: Remove the blank line.
Sign in to reply to this message.
http://codereview.appspot.com/189066/diff/1/7 File schedule_editor.py (right): http://codereview.appspot.com/189066/diff/1/7#newcode145 schedule_editor.py:145: """ Could you please rewrite StopToTuple() instead? Seems the only difference is the returned result. On 2010/01/14 08:06:37, calidion wrote: > On 2010/01/14 07:23:37, weiliu wrote: > > Why copied? Is it possible to inherit the function from viewer and add some > > other helper function to support the information that you need? > can not be done. >
Sign in to reply to this message.
updated due to unstable network, there are some invalid uploads. http://codereview.appspot.com/189066/diff/1011/19 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/189066/diff/1011/19#newcode42 gtfsscheduleviewer/files/transit_editor.js:42: linePoints[linePoints.length] = new GLatLng(points[i][0], points[i][1]); On 2010/01/15 04:11:33, baiming wrote: > Use .push() instead. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode122 gtfsscheduleviewer/files/transit_editor.js:122: gtfs.data.trips = {}; On 2010/01/15 04:11:33, baiming wrote: > Add jsDoc comments for trips, stops and stopsOfShape. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode273 gtfsscheduleviewer/files/transit_editor.js:273: * @param {String} tripId Trip ID of stoptimes to be fetched On 2010/01/15 04:11:33, baiming wrote: > Wrong parameter comment, please update it for "bounds". Done. http://codereview.appspot.com/189066/diff/1011/19#newcode280 gtfsscheduleviewer/files/transit_editor.js:280: + "&e=" + bounds.getNorthEast().lng() On 2010/01/15 04:11:33, baiming wrote: > Strange indentation. Use 4-char instead. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode860 gtfsscheduleviewer/files/transit_editor.js:860: if (Trip.current && Trip.current != data.trip) { On 2010/01/15 04:11:33, baiming wrote: > In which cases Trip.current != this.curTrip_? Done. http://codereview.appspot.com/189066/diff/1011/19#newcode946 gtfsscheduleviewer/files/transit_editor.js:946: * Reset Trip node to it's initial state On 2010/01/15 04:11:33, baiming wrote: > Besides the node and stops, the trip has many other statuses, "reset" doesn't > really reset all of them. So it still shouldn't has this name. > > Let's talk over phone about it. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1042 gtfsscheduleviewer/files/transit_editor.js:1042: On 2010/01/15 04:11:33, baiming wrote: > Remove these useless blank spaces. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1061 gtfsscheduleviewer/files/transit_editor.js:1061: Trip.prototype.resetStop = function(isTripSelected) { On 2010/01/15 04:11:33, baiming wrote: > "resetStop" implies it clears the trip's stop(s) to empty, but in fact it does > not. It only changes the stops' status. > > Rename it to resetStopStatus. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1200 gtfsscheduleviewer/files/transit_editor.js:1200: this.isTripSelected = false; On 2010/01/15 04:11:33, baiming wrote: > Shouldn't be private? Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1212 gtfsscheduleviewer/files/transit_editor.js:1212: Stop.add = function(stopInfo) { On 2010/01/15 04:11:33, baiming wrote: > Add comment. > > I like this static factory method, only a little comment on the name. According > to the static factory method's naming pattern, it commonly is named as "create", > "createXXX", "fromXXX" or "newXXX". I don't insist on the change though, please > give it a consideration. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1270 gtfsscheduleviewer/files/transit_editor.js:1270: * Create a labeled marker for the Stop object On 2010/01/15 04:11:33, baiming wrote: > Add @param for arrivalTime Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1288 gtfsscheduleviewer/files/transit_editor.js:1288: * Set marker text On 2010/01/15 04:11:33, baiming wrote: > Add @param for text. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1329 gtfsscheduleviewer/files/transit_editor.js:1329: Stop.prototype.clear = function() { On 2010/01/15 04:11:33, baiming wrote: > What is "clear" to clear? all statuses? or just the overlays? > If it only clears the overlays, please rename it. Let's try to avoid this type > of too general names. > > Talk about it over phone if you have questions. Done. http://codereview.appspot.com/189066/diff/1011/19#newcode1495 gtfsscheduleviewer/files/transit_editor.js:1495: /* On 2010/01/15 04:11:33, baiming wrote: > I just noticed that you make a small mistake in writing the jsDoc. > > The format of the comment is > /** (two * in the first line, instead of one) > * > */ Done.
Sign in to reply to this message.
in python code, we removed some useless member functions and override StopToTuple 2010/1/15 <calidion@gmail.com> > updated > due to unstable network, > there are some invalid uploads. > > > > http://codereview.appspot.com/189066/diff/1011/19 > File gtfsscheduleviewer/files/transit_editor.js (right): > > http://codereview.appspot.com/189066/diff/1011/19#newcode42 > gtfsscheduleviewer/files/transit_editor.js:42: > linePoints[linePoints.length] = new GLatLng(points[i][0], points[i][1]); > On 2010/01/15 04:11:33, baiming wrote: > >> Use .push() instead. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode122 > gtfsscheduleviewer/files/transit_editor.js:122: gtfs.data.trips = {}; > On 2010/01/15 04:11:33, baiming wrote: > >> Add jsDoc comments for trips, stops and stopsOfShape. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode273 > gtfsscheduleviewer/files/transit_editor.js:273: * @param {String} tripId > Trip ID of stoptimes to be fetched > On 2010/01/15 04:11:33, baiming wrote: > >> Wrong parameter comment, please update it for "bounds". >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode280 > gtfsscheduleviewer/files/transit_editor.js:280: + "&e=" + > bounds.getNorthEast().lng() > On 2010/01/15 04:11:33, baiming wrote: > >> Strange indentation. Use 4-char instead. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode860 > gtfsscheduleviewer/files/transit_editor.js:860: if (Trip.current && > Trip.current != data.trip) { > On 2010/01/15 04:11:33, baiming wrote: > >> In which cases Trip.current != this.curTrip_? >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode946 > gtfsscheduleviewer/files/transit_editor.js:946: * Reset Trip node to > it's initial state > On 2010/01/15 04:11:33, baiming wrote: > >> Besides the node and stops, the trip has many other statuses, "reset" >> > doesn't > >> really reset all of them. So it still shouldn't has this name. >> > > Let's talk over phone about it. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1042 > gtfsscheduleviewer/files/transit_editor.js:1042: > On 2010/01/15 04:11:33, baiming wrote: > >> Remove these useless blank spaces. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1061 > gtfsscheduleviewer/files/transit_editor.js:1061: > Trip.prototype.resetStop = function(isTripSelected) { > On 2010/01/15 04:11:33, baiming wrote: > >> "resetStop" implies it clears the trip's stop(s) to empty, but in fact >> > it does > >> not. It only changes the stops' status. >> > > Rename it to resetStopStatus. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1200 > gtfsscheduleviewer/files/transit_editor.js:1200: this.isTripSelected = > false; > On 2010/01/15 04:11:33, baiming wrote: > >> Shouldn't be private? >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1212 > gtfsscheduleviewer/files/transit_editor.js:1212: Stop.add = > function(stopInfo) { > On 2010/01/15 04:11:33, baiming wrote: > >> Add comment. >> > > I like this static factory method, only a little comment on the name. >> > According > >> to the static factory method's naming pattern, it commonly is named as >> > "create", > >> "createXXX", "fromXXX" or "newXXX". I don't insist on the change >> > though, please > >> give it a consideration. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1270 > gtfsscheduleviewer/files/transit_editor.js:1270: * Create a labeled > marker for the Stop object > On 2010/01/15 04:11:33, baiming wrote: > >> Add @param for arrivalTime >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1288 > gtfsscheduleviewer/files/transit_editor.js:1288: * Set marker text > On 2010/01/15 04:11:33, baiming wrote: > >> Add @param for text. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1329 > gtfsscheduleviewer/files/transit_editor.js:1329: Stop.prototype.clear = > function() { > On 2010/01/15 04:11:33, baiming wrote: > >> What is "clear" to clear? all statuses? or just the overlays? >> If it only clears the overlays, please rename it. Let's try to avoid >> > this type > >> of too general names. >> > > Talk about it over phone if you have questions. >> > > Done. > > > http://codereview.appspot.com/189066/diff/1011/19#newcode1495 > gtfsscheduleviewer/files/transit_editor.js:1495: /* > On 2010/01/15 04:11:33, baiming wrote: > >> I just noticed that you make a small mistake in writing the jsDoc. >> > > The format of the comment is >> /** (two * in the first line, instead of one) >> * >> */ >> > > Done. > > > http://codereview.appspot.com/189066 >
Sign in to reply to this message.
LGTM for python code. 2010/1/15 李白，字一日 <calidion@gmail.com> > in python code, we removed some useless member functions > and override StopToTuple > > 2010/1/15 <calidion@gmail.com> > > updated >> due to unstable network, >> there are some invalid uploads. >> >> >> >> http://codereview.appspot.com/189066/diff/1011/19 >> File gtfsscheduleviewer/files/transit_editor.js (right): >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode42 >> gtfsscheduleviewer/files/transit_editor.js:42: >> linePoints[linePoints.length] = new GLatLng(points[i][0], points[i][1]); >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Use .push() instead. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode122 >> gtfsscheduleviewer/files/transit_editor.js:122: gtfs.data.trips = {}; >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Add jsDoc comments for trips, stops and stopsOfShape. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode273 >> gtfsscheduleviewer/files/transit_editor.js:273: * @param {String} tripId >> Trip ID of stoptimes to be fetched >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Wrong parameter comment, please update it for "bounds". >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode280 >> gtfsscheduleviewer/files/transit_editor.js:280: + "&e=" + >> bounds.getNorthEast().lng() >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Strange indentation. Use 4-char instead. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode860 >> gtfsscheduleviewer/files/transit_editor.js:860: if (Trip.current && >> Trip.current != data.trip) { >> On 2010/01/15 04:11:33, baiming wrote: >> >>> In which cases Trip.current != this.curTrip_? >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode946 >> gtfsscheduleviewer/files/transit_editor.js:946: * Reset Trip node to >> it's initial state >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Besides the node and stops, the trip has many other statuses, "reset" >>> >> doesn't >> >>> really reset all of them. So it still shouldn't has this name. >>> >> >> Let's talk over phone about it. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1042 >> gtfsscheduleviewer/files/transit_editor.js:1042: >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Remove these useless blank spaces. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1061 >> gtfsscheduleviewer/files/transit_editor.js:1061: >> Trip.prototype.resetStop = function(isTripSelected) { >> On 2010/01/15 04:11:33, baiming wrote: >> >>> "resetStop" implies it clears the trip's stop(s) to empty, but in fact >>> >> it does >> >>> not. It only changes the stops' status. >>> >> >> Rename it to resetStopStatus. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1200 >> gtfsscheduleviewer/files/transit_editor.js:1200: this.isTripSelected = >> false; >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Shouldn't be private? >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1212 >> gtfsscheduleviewer/files/transit_editor.js:1212: Stop.add = >> function(stopInfo) { >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Add comment. >>> >> >> I like this static factory method, only a little comment on the name. >>> >> According >> >>> to the static factory method's naming pattern, it commonly is named as >>> >> "create", >> >>> "createXXX", "fromXXX" or "newXXX". I don't insist on the change >>> >> though, please >> >>> give it a consideration. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1270 >> gtfsscheduleviewer/files/transit_editor.js:1270: * Create a labeled >> marker for the Stop object >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Add @param for arrivalTime >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1288 >> gtfsscheduleviewer/files/transit_editor.js:1288: * Set marker text >> On 2010/01/15 04:11:33, baiming wrote: >> >>> Add @param for text. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1329 >> gtfsscheduleviewer/files/transit_editor.js:1329: Stop.prototype.clear = >> function() { >> On 2010/01/15 04:11:33, baiming wrote: >> >>> What is "clear" to clear? all statuses? or just the overlays? >>> If it only clears the overlays, please rename it. Let's try to avoid >>> >> this type >> >>> of too general names. >>> >> >> Talk about it over phone if you have questions. >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066/diff/1011/19#newcode1495 >> gtfsscheduleviewer/files/transit_editor.js:1495: /* >> On 2010/01/15 04:11:33, baiming wrote: >> >>> I just noticed that you make a small mistake in writing the jsDoc. >>> >> >> The format of the comment is >>> /** (two * in the first line, instead of one) >>> * >>> */ >>> >> >> Done. >> >> >> http://codereview.appspot.com/189066 >> > > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
http://codereview.appspot.com/189066/diff/65/1062 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/189066/diff/65/1062#newcode1 gtfsscheduleviewer/files/transit_editor.js:1: /*** ***? http://codereview.appspot.com/189066/diff/65/1062#newcode34 gtfsscheduleviewer/files/transit_editor.js:34: /*** Ditto. http://codereview.appspot.com/189066/diff/65/1062#newcode66 gtfsscheduleviewer/files/transit_editor.js:66: /*** Ditto. http://codereview.appspot.com/189066/diff/65/1062#newcode1228 gtfsscheduleviewer/files/transit_editor.js:1228: Stop.confirmAndGetStop = function(stopInfo) { Is getOrCreate a better name for this function? It's up to you to change or not. http://codereview.appspot.com/189066/diff/65/1062#newcode1338 gtfsscheduleviewer/files/transit_editor.js:1338: if(!this.latlng) { A space is needed between if and (. This problem widely exists in the file, please find and replace all "if(". http://codereview.appspot.com/189066/diff/65/1062#newcode1525 gtfsscheduleviewer/files/transit_editor.js:1525: for(var i = 0, stopInfo; stopInfo = data[i]; i++){ A space is needed between for and (. A space is needed between ) and {. Find and replace them.
Sign in to reply to this message.
updated http://codereview.appspot.com/189066/diff/65/1062 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/189066/diff/65/1062#newcode1 gtfsscheduleviewer/files/transit_editor.js:1: /*** On 2010/01/15 14:44:06, baiming wrote: > ***? Done. http://codereview.appspot.com/189066/diff/65/1062#newcode34 gtfsscheduleviewer/files/transit_editor.js:34: /*** On 2010/01/15 14:44:06, baiming wrote: > Ditto. Done. http://codereview.appspot.com/189066/diff/65/1062#newcode66 gtfsscheduleviewer/files/transit_editor.js:66: /*** On 2010/01/15 14:44:06, baiming wrote: > Ditto. Done. http://codereview.appspot.com/189066/diff/65/1062#newcode1228 gtfsscheduleviewer/files/transit_editor.js:1228: Stop.confirmAndGetStop = function(stopInfo) { On 2010/01/15 14:44:06, baiming wrote: > Is getOrCreate a better name for this function? > It's up to you to change or not. 'or' may not be so proper for the function always return a stop http://codereview.appspot.com/189066/diff/65/1062#newcode1338 gtfsscheduleviewer/files/transit_editor.js:1338: if(!this.latlng) { On 2010/01/15 14:44:06, baiming wrote: > A space is needed between if and (. > This problem widely exists in the file, please find and replace all "if(". Done. http://codereview.appspot.com/189066/diff/65/1062#newcode1525 gtfsscheduleviewer/files/transit_editor.js:1525: for(var i = 0, stopInfo; stopInfo = data[i]; i++){ On 2010/01/15 14:44:06, baiming wrote: > A space is needed between for and (. > A space is needed between ) and {. > > Find and replace them. Done.
Sign in to reply to this message.
LGTM. Though, you can take a look at the below comment and give a consideration to it. http://codereview.appspot.com/189066/diff/1071/69 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/189066/diff/1071/69#newcode1225 gtfsscheduleviewer/files/transit_editor.js:1225: * Confirm a Stop object to be created and return the stop by ID "getOrCreate" just indicates it either gets a created stop or creates a new one. In either case it will return a stop.
Sign in to reply to this message.
updated to rev 46. 2010/1/18 <baiming@google.com> > LGTM. > > Though, you can take a look at the below comment and give a > consideration to it. > > > http://codereview.appspot.com/189066/diff/1071/69 > > File gtfsscheduleviewer/files/transit_editor.js (right): > > http://codereview.appspot.com/189066/diff/1071/69#newcode1225 > gtfsscheduleviewer/files/transit_editor.js:1225: * Confirm a Stop object > to be created and return the stop by ID > "getOrCreate" just indicates it either gets a created stop or creates a > new one. In either case it will return a stop. > > http://codereview.appspot.com/189066 >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
