[READY] Use non-capturing group in JavaScript identifier regex #684

Merged
merged 2 commits into from Jan 7, 2017

Projects

None yet

5 participants

@micbou
Collaborator
micbou commented Jan 7, 2017 edited

In PR #643, we added a regular expression with a capturing group for JavaScript (and TypeScript) identifiers. Unfortunately, this makes re.findall returns matches of this group instead of the whole pattern. Since this function is used in ExtractIdentifiersFromText, it breaks the extraction of identifiers in JS and TS files. We fix the issue by replacing the (...) pattern with (:?...).

Added a test that fails without the fix:

AssertionError: ['var', 'foo', 'require', 'bar'] != ['v', 'f', 'r', 'b']

This change is Reviewable

micbou added some commits Jan 6, 2017
@micbou micbou Add ExtractIdentifiersFromText JavaScript test b0c41e3
@micbou micbou Fix JavaScript identifier regex
Capturing groups cannot be used in identifier regexes because it breaks
identifiers extraction from a text.
5ac921d
@codecov-io
codecov-io commented Jan 7, 2017 edited

Current coverage is 92.52% (diff: 100%)

Merging #684 into master will not change coverage

@@             master       #684   diff @@
==========================================
  Files            79         79          
  Lines          5166       5166          
  Methods         295        295          
  Messages          0          0          
  Branches        139        139          
==========================================
  Hits           4780       4780          
  Misses          330        330          
  Partials         56         56          

Powered by Codecov. Last update 96aabdc...5ac921d

@Valloric
Owner
Valloric commented Jan 7, 2017

:lgtm:

Nice, thanks!


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@puremourning
Collaborator

:lgtm: @homu r+


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@homu
Collaborator
homu commented Jan 7, 2017

📌 Commit 5ac921d has been approved by puremourning

@homu homu added a commit that referenced this pull request Jan 7, 2017
@homu homu Auto merge of #684 - micbou:fix-js-identifier-regex, r=puremourning
[READY] Use non-capturing group in JavaScript identifier regex

In PR #643, we added a regular expression with a capturing group for JavaScript (and TypeScript) identifiers. Unfortunately, this makes [`re.findall` returns matches of this group instead of the whole pattern](https://docs.python.org/2/library/re.html#re.findall). Since this function is used in `ExtractIdentifiersFromText`, it breaks the extraction of identifiers in JS and TS files. We fix the issue by replacing the `(...)` pattern with `(:?...)`.

Added a test that fails without the fix:
```
AssertionError: ['var', 'foo', 'require', 'bar'] != ['v', 'f', 'r', 'b']
```

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

⌛️ Testing commit 5ac921d with merge 0e7ad1f...

@homu
Collaborator
homu commented Jan 7, 2017

☀️ Test successful - status

@homu homu merged commit 5ac921d into Valloric:master Jan 7, 2017

7 checks passed

code-review/reviewable Review complete: 2 of 2 LGTMs obtained
Details
codecov/changes No unexpected coverage changes found.
Details
codecov/patch Coverage not affected when comparing 96aabdc...5ac921d
Details
codecov/project 92.52% (+0.00%) compared to 96aabdc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@puremourning puremourning referenced this pull request in Valloric/YouCompleteMe Jan 7, 2017
Merged

[READY] Update readme for compilation database support #2495

4 of 4 tasks complete
@homu homu added a commit to Valloric/YouCompleteMe 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment