----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3564/#review4646 -----------------------------------------------------------
./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10314> Please add the year to the header. ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10315> Please follow this template for specifying author: @author [email protected] (Yuri Zelikov) ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10316> Please make sure all variable names follow the standard java conventions, i.e. raw_deltas -> rawDeltas ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10318> I think it would be better if instead of describing the with a comment, you would extract the code snippet into a method. For example we can extract this snippet into a new method createImportedWavelet() or some similar name. ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10317> current_version -> currentVersion from_version -> fromVersion ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10319> Again, probably would be better to extract this code into applyDeltasToWave() method. ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10320> applied_deltas -> appliedDeltas ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10321> applied_delta -> appliedDelta ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10322> Please replace it with just a TODO // TODO (Andrew Kaplanov) Implement attachments import. Issue <link to issue in Apache Wave Jira> You can open a new issue in the Jira and attach there the commented code instead of putting it here. In general no commented code should be committed. ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10323> It seems like the line is too long. Please make sure that the line length does not exceed 100 chars ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10324> Line too long ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10325> Can we include some more info in the log message? Like wave id, wavelet id etc... ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10326> WIAB has the so called "shared participant" (@yourwavedomain.com), which functions similar to the [email protected]. Can we handle this case by adding the shared participant instead of it? ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java <https://reviews.apache.org/r/3564/#comment10327> Line too long. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10328> Add year to the header ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10329> Fix @author to comply with template + remove empty line from above. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10336> Add some javadoc to the class describing its purpose. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10330> Is this parameter general? If not we need to init it from properties. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10331> Remove trailing spaces. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10332> I think it should be initialized from the properties. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10333> Same ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10334> Remove trailing spaces. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10335> Remove trailing spaces. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10337> Is there any reason for these variables not to be "final"? ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10339> Please fix indentation in this class - should be two spaces. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10338> Please put all looped code inside a block {} ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10340> Remove trailing space. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10341> Remove trailing space. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10342> Remove trailing space. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10343> Remove trailing space. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10344> Remove trailing space. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10346> Make the API RPC string a constant private static final String ... ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10348> wave_num - > waveNum ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10349> processed_count -> processedCount ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10350> not_processed_count -> notProcessedCount ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10351> Why not while(true) ? IMO it is clearer that for (;;) ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10345> Can we make the after and before configurable? Or at least make them class constants. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10347> Please make sure to put all code in the "if" into a block {} - even id it's only one line. ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10352> Shouldn't we log some warning here? ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10353> Line too long ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10354> Can we extract this snippet that writes to a file into a method? ./src/org/waveprotocol/box/waveimport/WaveExport.java <https://reviews.apache.org/r/3564/#comment10355> Please define a static LOG variable and use it instead of Logger.getLogger() ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10356> Add year to the header. ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10357> Fix @author + add some javadoc for the class. ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10358> Can we make the name of this method self descriptive instead of adding a comment? imp() -> importWavesFromFiles() ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10359> Same, please follow Java conventions for variable names. ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10360> wave_id -> waveId ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10361> Can you provide some details why it is needed? ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10362> It seems like this method return value has nothing to do with what it does. It might be convenient to use, but it obfuscates what it does. Can we maybe throw a ImportRequestSkippedException in case the request was skipped and change the return type of this method to void? ./src/org/waveprotocol/box/waveimport/WaveImport.java <https://reviews.apache.org/r/3564/#comment10363> for(;;) -> while(true) ./src/org/waveprotocol/box/waveimport/google/RobotApi.java <https://reviews.apache.org/r/3564/#comment10364> Please follow the same template for the @author ./src/org/waveprotocol/box/waveimport/google/RobotApi.java <https://reviews.apache.org/r/3564/#comment10365> log -> LOG ./src/org/waveprotocol/box/waveimport/google/RobotApi.java <https://reviews.apache.org/r/3564/#comment10366> Remove trailing space ./src/org/waveprotocol/box/waveimport/google/RobotApi.java <https://reviews.apache.org/r/3564/#comment10367> Remove trailing space ./src/org/waveprotocol/box/waveimport/google/RobotApi.java <https://reviews.apache.org/r/3564/#comment10368> Remove empty line ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestImpl.java <https://reviews.apache.org/r/3564/#comment10369> Remove empty line ./src/org/waveprotocol/box/waveimport/google/google-import.proto <https://reviews.apache.org/r/3564/#comment10370> Why do we nee this proto? Do we compile it into corresponding java class? - Yuri On 2012-01-26 11:34:03, Andrew Kaplanov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3564/ > ----------------------------------------------------------- > > (Updated 2012-01-26 11:34:03) > > > Review request for wave and Yuri Zelikov. > > > Summary > ------- > > Exports waves from GWave to files, and imports them to Wiab by two command > line utilities. > The implementation is very raw, due to time constraints. > Import accesses special service on the Wiab. > Authorization on import service are not implemented now. > Imports of attachments not yet implemented. > Also, there is no generation of wave Id's for import into a non-empty store, > and from different domains. > Instructions for use enclosed in the file README.import. > > > Diffs > ----- > > ./README.import PRE-CREATION > ./build.properties 1235719 > ./build.xml 1235719 > ./run-export.sh PRE-CREATION > ./run-import.sh PRE-CREATION > ./src/org/waveprotocol/box/server/ServerMain.java 1235719 > ./src/org/waveprotocol/box/server/waveserver/ImportServlet.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/WaveExport.java PRE-CREATION > ./src/org/waveprotocol/box/waveimport/WaveImport.java PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/RobotApi.java PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigest.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestGsonImpl.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestImpl.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/RobotSearchDigestUtil.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/google-import.proto > PRE-CREATION > > ./src/org/waveprotocol/box/waveimport/google/oauth/NeedNewOAuthTokenException.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/oauth/OAuthCredentials.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/oauth/OAuthRequestHelper.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/oauth/OAuthedFetchService.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/oauth/StableUserId.java > PRE-CREATION > ./src/org/waveprotocol/box/waveimport/google/oauth/UserContext.java > PRE-CREATION > ./third_party/runtime/google_client/COPYING PRE-CREATION > ./third_party/runtime/google_client/google-api-client-1.5.0-beta.jar > UNKNOWN > ./third_party/runtime/google_client/google-http-client-1.5.0-beta.jar > UNKNOWN > ./third_party/runtime/google_client/google-oauth-client-1.5.0-beta.jar > UNKNOWN > ./third_party/runtime/google_client/lucene-core-3.5.0-javadoc.jar UNKNOWN > ./third_party/runtime/google_client/lucene-core-3.5.0.jar UNKNOWN > > Diff: https://reviews.apache.org/r/3564/diff > > > Testing > ------- > > > Thanks, > > Andrew > >
