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

Issue 291280043: Principal is domainFormat agnostic.

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 8 months ago by afedorenchik
Modified:
1 week, 5 days ago
Reviewers:
myk
CC:
connector-cr_google.com
Visibility:
Public.

Description

Principal is domainFormat agnostic.

Patch Set 1 #

Total comments: 11

Patch Set 2 : Principal is domainFormat agnostic. #

Patch Set 3 : Principal is domainFormat agnostic. #

Total comments: 2

Patch Set 4 : Principal is domainFormat agnostic. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -46 lines) Patch
M src/com/google/enterprise/adaptor/Principal.java View 1 2 4 chunks +18 lines, -7 lines 0 comments Download
M test/com/google/enterprise/adaptor/GsaFeedFileMakerTest.java View 9 chunks +32 lines, -32 lines 0 comments Download
M test/com/google/enterprise/adaptor/PrincipalTest.java View 1 2 3 4 chunks +17 lines, -7 lines 0 comments Download

Messages

Total messages: 5
afedorenchik
6 years, 8 months ago (2016-03-24 15:36:12 UTC) #1
myk
The changes to Principal.java look great -- but while you have made tiny changes to ...
6 years, 8 months ago (2016-03-24 16:03:28 UTC) #2
afedorenchik
Hi, I've done changes and all tests are good. Please let me know if you ...
6 years, 8 months ago (2016-03-24 18:55:17 UTC) #3
myk
Looks good to me. Thank you very much for your work! https://codereview.appspot.com/291280043/diff/1/src/com/google/enterprise/adaptor/Principal.java File src/com/google/enterprise/adaptor/Principal.java (right): ...
6 years, 8 months ago (2016-03-24 21:08:12 UTC) #4
afedorenchik
6 years, 8 months ago (2016-03-24 21:56:24 UTC) #5
https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/PrincipalTest.java (right):

https://codereview.appspot.com/291280043/diff/40001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/PrincipalTest.java:146:
assertFalse(p6.equals(g1));
On 2016/03/24 21:08:12, myk wrote:
> Above new tests look good!  I'm surprised that Eclipse let you put in a line
> with spaces only (lines 137, 141, 143).  It's OK with me if you leave those
> spaces in (or if you decide to delete them).  If you do delete them (but make
no
> other change), feel free to submit.

Acknowledged. Not sure if it OK to leave as is. I can remove this spaces. Also
confused why Eclipse does not removed them on save.
Sign in to reply to this message.

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