Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1108)

Issue 99660045: make path public on PathObserver and change third argument from path to the PathObserver instance (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 10 months ago by rafaelw
Modified:
9 years, 10 months ago
Reviewers:
arv, rafaelw1, John Messerly
Base URL:
git@github.com:Polymer/observe-js.git@master
Visibility:
Public.

Description

make path public on PathObserver and change third argument from path to the PathObserver instance R=jmesserly@google.com, jmesserly BUG= Committed: https://github.com/Polymer/observe-js/commit/fe4b09b

Patch Set 1 #

Total comments: 2

Patch Set 2 : make path a getter #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -2 lines) Patch
M src/observe.js View 1 2 chunks +5 lines, -1 line 0 comments Download
M tests/test.js View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 9
John Messerly
lgtm
9 years, 10 months ago (2014-05-29 22:34:21 UTC) #1
arv
https://codereview.appspot.com/99660045/diff/1/src/observe.js File src/observe.js (right): https://codereview.appspot.com/99660045/diff/1/src/observe.js#newcode1000 src/observe.js:1000: this.path = getPath(path); How about keeping path_ and add ...
9 years, 10 months ago (2014-05-29 22:44:08 UTC) #2
John Messerly
https://codereview.appspot.com/99660045/diff/1/src/observe.js File src/observe.js (right): https://codereview.appspot.com/99660045/diff/1/src/observe.js#newcode1000 src/observe.js:1000: this.path = getPath(path); On 2014/05/29 22:44:08, arv wrote: > ...
9 years, 10 months ago (2014-05-29 22:49:56 UTC) #3
rafaelw1
sold. On Thu, May 29, 2014 at 3:49 PM, <jmesserly@google.com> wrote: > > https://codereview.appspot.com/99660045/diff/1/src/observe.js > ...
9 years, 10 months ago (2014-05-29 22:52:57 UTC) #4
rafaelw
Committed patchset #2 manually as rfe4b09b (presubmit successful).
9 years, 10 months ago (2014-05-29 22:53:23 UTC) #5
arv
LGTM No test for path?
9 years, 10 months ago (2014-05-29 23:03:51 UTC) #6
rafaelw1
Do you want something other than what's in the existing path (like asserting that it's ...
9 years, 10 months ago (2014-05-30 17:52:13 UTC) #7
arv
The test can be trivial. I just think it is good practice to add tests ...
9 years, 10 months ago (2014-05-30 18:14:49 UTC) #8
rafaelw1
9 years, 10 months ago (2014-05-30 18:30:28 UTC) #9
Fair enough:
https://github.com/Polymer/observe-js/commit/d356c7aceb137ac02955dfbca0288319...


On Fri, May 30, 2014 at 11:14 AM, Erik Arvidsson <arv@chromium.org> wrote:

> The test can be trivial. I just think it is good practice to add tests for
> new public APIs.
>
>
> On Fri, May 30, 2014 at 1:52 PM, Rafael Weinstein <rafaelw@google.com>
> wrote:
>
>> Do you want something other than what's in the existing path (like
>> asserting that it's read-only?).
>>
>>
>> On Thu, May 29, 2014 at 4:03 PM, <arv@chromium.org> wrote:
>>
>>> LGTM
>>>
>>> No test for path?
>>>
>>> https://codereview.appspot.com/99660045/
>>>
>>
>>
>
>
> --
> erik
>
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b