[READY] Update readme for compilation database support #2495

Merged
merged 3 commits into from Jan 8, 2017

Projects

None yet

6 participants

@puremourning
Collaborator
puremourning commented Jan 7, 2017 edited

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.

only changes docs

  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

This change:

  • updates the c-family completer documentation to describe the built in support for compilation databases added in Valloric/ycmd#680
  • explains more about why ycmd needs compiler flags, and how to go about providing them
  • recommends using a compilation database (as that seems to be the fashion)
  • standardises formatting for NOTE (it was inconsistent before)
  • states that the preferred installation method is install.py (rather than the full installation instructions)
  • update the vim doc
  • update the ycmd submodule

ycmd update release note


This change is Reviewable

@Valloric
Owner
Valloric commented Jan 7, 2017

:lgtm:

Great docs, thanks!


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


README.md, line 814 at r1 (raw file):

### C-family Semantic Completion

In order to perform semantic analysis such as code completion, goto and

You might want to capitalize goto as GoTo so it's more obvious you're talking about a command.


README.md, line 910 at r1 (raw file):

As you can see from the trivial example, YCM calls the `FlagsForFile` method,
passing it the file name. The `**kwargs` is for advanced users and can be
ignored for many users. The `FlagsForFile` function returns a dictionary with a

Reword this to "and can usually be ignored."?


Comments from Reviewable

@codecov-io
codecov-io commented Jan 7, 2017 edited

Current coverage is 85.66% (diff: 100%)

Merging #2495 into master will not change coverage

@@             master      #2495   diff @@
==========================================
  Files            18         18          
  Lines          1912       1912          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1638       1638          
  Misses          274        274          
  Partials          0          0          

Powered by Codecov. Last update f34b39d...3eb8a33

@puremourning puremourning changed the title from [RFC] Update readme for compilation database support to [READY] Update readme for compilation database support Jan 7, 2017
@puremourning puremourning changed the title from [READY] Update readme for compilation database support to [RFC] Update readme for compilation database support Jan 7, 2017
@puremourning puremourning changed the title from [RFC] Update readme for compilation database support to [READY] Update readme for compilation database support Jan 7, 2017
@puremourning
Collaborator

OK updated ycmd and the vim docs. This is ready.

@puremourning
Collaborator

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.


README.md, line 814 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

You might want to capitalize goto as GoTo so it's more obvious you're talking about a command.

Done.


README.md, line 910 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Reword this to "and can usually be ignored."?

Done.


Comments from Reviewable

@vheon
Collaborator
vheon commented Jan 7, 2017

:lgtm:


Reviewed 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@micbou
Collaborator
micbou commented Jan 7, 2017

That's awesome documentation. Made minor comments but :lgtm:.


Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


README.md, line 834 at r1 (raw file):

- If using CMake, add `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON` when configuring and
  copy or symlink it to the root of your project.

Should we rather suggest to add the line set( CMAKE_EXPORT_COMPILE_COMMANDS ON ) in CMakeLists.txt? Technically, this is doing the same thing but I feel it's more convenient than passing a flag to the cmake command.


README.md, line 837 at r1 (raw file):

- If using Ninja, check out the `compdb` tool (`-t compdb`) in its
  [docs][ninja-compdb]
- If using GNU make, check out [Bear][]

Missing dot at the end of line for these two items.


README.md, line 853 at r1 (raw file):

- If the database contains an entry for the file, the flags for that file are
  used.
- If the file is a header file and a source file with he same root exists in the

The instead of he


README.md, line 816 at r3 (raw file):

In order to perform semantic analysis such as code completion, `GoTo` and
diagnostics, YouCompleteMe uses `libclang`. This is the library version of the
clang compiler, sometimes also referred to as llvm. Like any compiler,

Sorry for being pedantic but I think we should always write proper nouns with their capital letters: LLVM instead of llvm, LibClang instead of libclang, C++ instead of c++, etc.


README.md, line 904 at r3 (raw file):

def FlagsForFile( filename, **kwargs ):
  return {
    'flags': '-x c++ -Wall -Wextra -Werror'.split(),

I am not sure that's a good idea to encourage people to use split(). This will fail with spaces in paths.


Comments from Reviewable

@Valloric
Owner
Valloric commented Jan 8, 2017

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


README.md, line 816 at r3 (raw file):

Previously, micbou wrote…

Sorry for being pedantic but I think we should always write proper nouns with their capital letters: LLVM instead of llvm, LibClang instead of libclang, C++ instead of c++, etc.

Fine with all of these except LibClang instead of libclang; the lowercase version is the name the libclang project uses.


Comments from Reviewable

@micbou
Collaborator
micbou commented Jan 8, 2017

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


README.md, line 816 at r3 (raw file):

Previously, Valloric (Val Markovic) wrote…

Fine with all of these except LibClang instead of libclang; the lowercase version is the name the libclang project uses.

Right, the capitalized version is almost never used. Let's keep libclang then.


Comments from Reviewable

@micbou micbou referenced this pull request Jan 8, 2017
Closed

Identifier completer not working #2497

6 of 11 tasks complete
@puremourning
Collaborator

All done, and thanks again for the reviews!


Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions.


README.md, line 834 at r1 (raw file):

Previously, micbou wrote…

Should we rather suggest to add the line set( CMAKE_EXPORT_COMPILE_COMMANDS ON ) in CMakeLists.txt? Technically, this is doing the same thing but I feel it's more convenient than passing a flag to the cmake command.

I think I prefer this one because it works for a larger number of use-cases. The case I'm thinking of (which is the one that I personally use it for most) is where changing the CMakeLists.txt is not really part of what I want to do (it would be a separate change to the thing I'm interested in). I think preferring this method lowers the barrier to entry.

In any case, I've added it as an alternative to cover all bases :)


README.md, line 837 at r1 (raw file):

Previously, micbou wrote…

Missing dot at the end of line for these two items.

Done.


README.md, line 853 at r1 (raw file):

Previously, micbou wrote…

The instead of he

Done.


README.md, line 904 at r3 (raw file):

Previously, micbou wrote…

I am not sure that's a good idea to encourage people to use split(). This will fail with spaces in paths.

OK, i thought this was simpler to read for non-pythonners. But you're right, it's just a potential source of problems. Most programmers can understand lists, right?


Comments from Reviewable

@micbou
Collaborator
micbou commented Jan 8, 2017

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


README.md, line 905 at r4 (raw file):

def FlagsForFile( filename, **kwargs ):
  return {
    'flags': [ '-x', 'c++', '-Wall',  '-Wextra', '-Werror' ],

Two spaces between -Wall and -Wextra.


Comments from Reviewable

puremourning added some commits Jan 2, 2017
@puremourning puremourning Update readme for compilation database support a8261e9
@puremourning puremourning Update ycmd 19c66a6
@puremourning puremourning Update vim documentaion
3eb8a33
@puremourning
Collaborator

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


README.md, line 905 at r4 (raw file):

Previously, micbou wrote…

Two spaces between -Wall and -Wextra.

Good spot. Done.


Comments from Reviewable

@micbou
Collaborator
micbou commented Jan 8, 2017

@homu r+


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@homu
Collaborator
homu commented Jan 8, 2017

📌 Commit 3eb8a33 has been approved by micbou

@homu homu merged commit 3eb8a33 into Valloric:master Jan 8, 2017

5 of 6 checks passed

code-review/reviewable Review in progress: 0 of 2 LGTMs obtained (and 3 stale)
Details
codecov/changes No unexpected coverage changes found.
Details
codecov/patch Coverage not affected when comparing f34b39d...3eb8a33
Details
codecov/project 85.66% (+0.00%) compared to f34b39d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@homu homu added a commit that referenced this pull request Jan 8, 2017
@homu homu Auto merge of #2495 - puremourning:readme-compilation-database, r=micbou
[READY] Update readme for compilation database support

# PR Prelude

Thank you for working on YCM! :)

**Please complete these steps and check these boxes (by putting an `x` inside
the brackets) _before_ filing your PR:**

- [X] I have read and understood YCM's [CONTRIBUTING][cont] document.
- [X] I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
- [X] I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.

> only changes docs

- [X] **I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.**

# Why this change is necessary and useful

This change:
 - updates the c-family completer documentation to describe the built in support for compilation databases added in Valloric/ycmd#680
 - explains more about why ycmd needs compiler flags, and how to go about providing them
 - recommends using a compilation database (as that seems to be the fashion)
 - standardises formatting for `NOTE` (it was inconsistent before)
 - states that the preferred installation method is `install.py` (rather than the full installation instructions)
 - update the vim doc
 - update the ycmd submodule

### ycmd update release note

- Valloric/ycmd#678 - Bump Boost version to 1.63.0
- Valloric/ycmd#686 - Update JediHTTP for Python 3.6 support
- Valloric/ycmd#684 - Fix JavaScript identifier regex
- Valloric/ycmd#680 - Automatically load a compilation database if found

[cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md
[code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2495)
<!-- Reviewable:end -->
d02de4b
@homu
Collaborator
homu commented Jan 8, 2017

⚡️ Test exempted - status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment