Bug 119618 - Adding a .ycm_extra_conf file for webkitGtk
Summary: Adding a .ycm_extra_conf file for webkitGtk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-09 03:04 PDT by Danilo de Paula
Modified: 2013-12-13 00:13 PST (History)
3 users (show)

See Also:


Attachments
Patch (7.32 KB, patch)
2013-08-09 03:07 PDT, Danilo de Paula
no flags Details | Formatted Diff | Diff
Iterated version of Danilo's patch (9.35 KB, patch)
2013-12-11 16:39 PST, Martin Robinson
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Danilo de Paula 2013-08-09 03:04:30 PDT
YouCompleteMe is a vim plugin to allow semantic auto completion (and other fetures) on c/c++ code.

It was originally built for chromium and it requires special configurations[1] depending on the build system.
I have been using it for a while now, and it might be handy for others webkitgtk developers.


[1] https://github.com/scheib/chromium/blob/master/tools/vim/chromium.ycm_extra_conf.py
Comment 1 Danilo de Paula 2013-08-09 03:07:32 PDT
Created attachment 208411 [details]
Patch
Comment 2 Martin Robinson 2013-12-11 16:39:20 PST
Created attachment 219016 [details]
Iterated version of Danilo's patch

I've worked on Danilo's patch a bit to make it conform to our style guidelines. YCM is great!
Comment 3 Adrian Perez 2013-12-12 10:21:37 PST
I have just a couple of nits in this informal review. Apart from those, the
script is great and YCM is a really welcome addition to Vim (I did not know
about it before today :D)

> Tools/gtk/ycm_extra_conf.py:30
> +sys.path = [__tools_directory] + sys.path

It would be good to use os.path.abspath() after determining
__tools_directory, just in case.

> Tools/gtk/ycm_extra_conf.py:113
> +def FlagsForFile(filename):

According to the YouCompleteMe documentation, FlagsForFile may be passed
keyword arguments, so this should be:

  def FlagsForFile(filename, **kwargs)

Even if the keyword arguments are unused for the moment, it is good to
include them in the function signature just in case users have set an
option in their vimrc to send extra arguments to YCM.
Comment 4 Gustavo Noronha (kov) 2013-12-12 10:40:10 PST
Comment on attachment 219016 [details]
Iterated version of Danilo's patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219016&action=review

LGTM, with a small refactoring proposal for the first function

> Tools/gtk/ycm_extra_conf.py:47
> +            for flag in FLAGS_PRECEDING_PATHS:

This for block could be replaced by if argument in FLAGS_PRECEDING_PATHS, you could always ensure argument is either a flag or a path to have a single code path here, it will be much more readable.

> Tools/gtk/ycm_extra_conf.py:54
> +                # Some argument contain the flag and the path together. For these

Missing last bit of the comment?

> Tools/gtk/ycm_extra_conf.py:101
> +        print "==== Error during reading %s file", trace_file_path

s/during/while/?
Comment 5 Martin Robinson 2013-12-12 15:16:20 PST
(In reply to comment #4)
> (From update of attachment 219016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219016&action=review
> 
> LGTM, with a small refactoring proposal for the first function
> 
> > Tools/gtk/ycm_extra_conf.py:47
> > +            for flag in FLAGS_PRECEDING_PATHS:
> 
> This for block could be replaced by if argument in FLAGS_PRECEDING_PATHS, you could always ensure argument is either a flag or a path to have a single code path here, it will be much more readable.

The arguments may be of the form -I /path or --argument=path. In the former case, we have to iterate the list of arguments to do a bunch of string searches. I did consider breaking out the  argument in FLAGS_PRECEDING_PATHS case like this:

for argument in arguments:
    if make_next_absolute:
        ...
    elif argument in FLAGS_PRECEDING_PATHS:
        make_next_absolute = True
    else:
        for flag in FLAGS_PRECEDING_PATHS:
            if argument.startswith(flag):
                ...

I went with the current approach because it was a little bit more efficient. I like both approaches though, so I can use whichever you prefer. :)

> 
> > Tools/gtk/ycm_extra_conf.py:54
> > +                # Some argument contain the flag and the path together. For these
> 
> Missing last bit of the comment?

Yes! Thanks.

> 
> > Tools/gtk/ycm_extra_conf.py:101
> > +        print "==== Error during reading %s file", trace_file_path
> 
> s/during/while/?

Thanks. I missed this.
Comment 6 Martin Robinson 2013-12-12 15:16:56 PST
(In reply to comment #3)
> I have just a couple of nits in this informal review. Apart from those, the
> script is great and YCM is a really welcome addition to Vim (I did not know
> about it before today :D)

Thanks for the review!


> > Tools/gtk/ycm_extra_conf.py:30
> > +sys.path = [__tools_directory] + sys.path
> 
> It would be good to use os.path.abspath() after determining
> __tools_directory, just in case.

Okay.


> > Tools/gtk/ycm_extra_conf.py:113
> > +def FlagsForFile(filename):
> 
> According to the YouCompleteMe documentation, FlagsForFile may be passed
> keyword arguments, so this should be:
> 
>   def FlagsForFile(filename, **kwargs)
> 
> Even if the keyword arguments are unused for the moment, it is good to
> include them in the function signature just in case users have set an
> option in their vimrc to send extra arguments to YCM.

Good idea. I'll make the change.
Comment 7 Adrian Perez 2013-12-12 15:38:12 PST
(In reply to comment #6)
> (In reply to comment #3)
> 
> > > Tools/gtk/ycm_extra_conf.py:30
> > > +sys.path = [__tools_directory] + sys.path
> > 
> > It would be good to use os.path.abspath() after determining
> > __tools_directory, just in case.
> 
> Okay.

Also, I forgot to mention that this looks like a more Pythonic way of
modifying an array (and it avoids creating temporary values):

  sys.path.insert(0, __tools_directory)
Comment 8 Martin Robinson 2013-12-13 00:13:39 PST
Committed r160531: <http://trac.webkit.org/changeset/160531>