incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ali Lown" <...@lown.me.uk>
Subject Re: Review Request 16322: full text search (with known issue of solr-bot output highlight)
Date Sat, 21 Dec 2013 14:49:36 GMT


> 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.
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message