> On Dec. 21, 2013, 11 a.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 55 > > <https://reviews.apache.org/r/16322/diff/2/?file=398901#file398901line55> > > > > Does this comply with the code style reinforced here? > > > > http://maven.apache.org/developers/conventions/code.html > > > > I don't think this is respected. > > Blocks: Always enclose with a new line brace. > > > > I used to have http://maven.apache.org/plugins/maven-checkstyle-plugin/ > > > > I don't think some rules make sense at all. > > > > Anyway, where can I read about the rules? Thanks. > > > > p.s. sorry for the delay in response. I'm still a learner to the review > > site. > > Yuri Zelikov wrote: > I think you are right, we need to decide on code style for Wave. > Basically we follow the standard Eclipse 2.1 built in style conventions with > following additions: > Use 2 spaces for indentation(not tabs) > Max line length is 100. > > Maybe some more but I can't recall the rest right now. > > Also it is advised to enable the 'Save Action" in Eclipse with following > options: > Format source code (edited lines) > Organize imports > Additional actions -> add final modifier to private fields > > Ali Lown wrote: > Yuri's comment here is referring to the trailing whitespace, which should > not be here. > > We don't have a single document stating the coding convention (and the > codebase is not 100% self consistent). > The best reference is the "style" of the code in the surrounding parts of > the file being edited. > > Frank R. wrote: > That little trailing space is introduced by eclipse whenever I press > CTRL+SHIF+F. I do that more frequently than CTRL+S :) > > I'll try to tweak my Java Formatter profile as suggested by Yuri. I did > try to find where to disable the trailing space, but I had no luck. > > It seems eclipse can adopt itself to the existing code at least with the > indention of two spaces. > > Eclipse used to reduce successive newlines into one on formatting. Don't > when that behavior changed to preserving two. > > --- > > I found the settings here, .settings/org.eclipse.jdt.core.prefs when I > was trying to creating a formatter profile, and config the save actions. > > This should be changed to 1. Right? > org.eclipse.jdt.core.formatter.number_of_empty_lines_to_preserve=2 > > Settings Yuri talked about: > > org.eclipse.jdt.core.formatter.tabulation.char=space > org.eclipse.jdt.core.formatter.tabulation.size=2 > > org.eclipse.jdt.core.formatter.lineSplit=100 > > Save Action goes here, .settings/org.eclipse.jdt.ui.prefs, which doesn't > include the desired settings: > > sp_cleanup.format_source_code=true > sp_cleanup.format_source_code_changes_only=true > > sp_cleanup.organize_imports=true > > sp_cleanup.make_private_fields_final=true > > There are more settings added when I set the rules up in Project > Properties. > Format source code (edited lines) > Organize imports > Additional actions -> add final modifier to private fields (I cleared > other default actions.) > > Can you, Ali and Yuri, submit an updated > .settings/org.eclipse.jdt.core.prefs and .settings/org.eclipse.jdt.ui.prefs > to match the rules you in your mind? Thanks~
Please see https://reviews.apache.org/r/16434/ > On Dec. 21, 2013, 11 a.m., Frank R. wrote: > > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java, line 178 > > <https://reviews.apache.org/r/16322/diff/6/?file=401693#file401693line178> > > > > Thanks for showing me the constants. I'll change their scope from > > private to public. > > > > Can you explain more what you mean by "these"? Thanks > > Ali Lown wrote: > Rather than just changing the visibility of the constants in > wave.client.doodad.selection.SelectionAnnotationHandler, and then referring > to them from there (which would be pretty ugly). > > I was requesting that you move the constants out of > wave.client.doodad.selection.SelectionAnnotationHandler, in to a new file in > the wave.model.conversation namespace, and then update both your code, and > SelectionAnnotationHandler to refer to the constants from there. > > Frank R. wrote: > I was trying to make as little impact as possible to existing code > written by others. I can do that some day. Now, I would suggest someone like > you with more experience in wave to go ahead and make the changes with a > bigger picture in mind. Thanks~ Fine. I will do this moving, and fix the BACKGROUND_COLOUR at the same time. I should create a review request within the next half hour. - Ali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16322/#review30770 ----------------------------------------------------------- On Dec. 20, 2013, 5:12 p.m., Frank R. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16322/ > ----------------------------------------------------------- > > (Updated Dec. 20, 2013, 5:12 p.m.) > > > Review request for wave, Ali Lown, Vicente J. Ruiz Jurado, and Yuri Zelikov. > > > Bugs: WAVE-311 > https://issues.apache.org/jira/browse/WAVE-311 > > > Repository: wave > > > Description > ------- > > For details (issues and commits): > https://github.com/renfeng/wave > > > Diffs > ----- > > run-export.sh d2cddb7 > run-import.sh 45fff8a > server.config.example 19ba8b2 > src/org/waveprotocol/box/server/SearchModule.java 2de0ef9 > src/org/waveprotocol/box/server/ServerMain.java b50454d > src/org/waveprotocol/box/server/robots/agent/search/SolrRobot.java > PRE-CREATION > src/org/waveprotocol/box/server/robots/agent/welcome/WelcomeRobot.java > 2735940 > src/org/waveprotocol/box/server/waveserver/SimpleSearchProviderImpl.java > ee7093f > src/org/waveprotocol/box/server/waveserver/SolrSearchProviderImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/SolrWaveIndexerImpl.java > PRE-CREATION > src/org/waveprotocol/box/server/waveserver/WaveDigester.java b103bbb > > Diff: https://reviews.apache.org/r/16322/diff/ > > > Testing > ------- > > tests on search box > > * in:inbox > * (empty) for all, including waves shared in the domain > * with:@ > * (free texts) > > tests on solr-bot > > * single word > * phrase (quoted with double quotation marks) > * syntax applicable to search box > > > Thanks, > > Frank R. > >
