Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove IdentifierEndsWith() declaration #732

Merged
merged 1 commit into from
Mar 31, 2017

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Mar 30, 2017

Just like @oblitum pointed out in #701(comment), in CompletionData.cpp the IdentifierEndsWith() function is called only from function RemoveTrailingParens() and both functions are in unnamed namespace, so neither should have declaration in CompletionData.h.

@oblitum, in his comment, mentions that he had noticed more odd changes in that PR. While this commit fixes the change he explicitely mentioned I would like to wait and see if he has to add anything else.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Mar 30, 2017

Codecov Report

Merging #732 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #732   +/-   ##
=======================================
  Coverage   93.16%   93.16%           
=======================================
  Files          79       79           
  Lines        5267     5267           
  Branches      155      155           
=======================================
  Hits         4907     4907           
  Misses        301      301           
  Partials       59       59

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ef1ae0...824a36c. Read the comment docs.

@Valloric
Copy link
Member

:lgtm:

Thanks for the PR!

I'm also fine with submitting this as-is and handling separate PRs for any future issues.

@bstaletic
Copy link
Collaborator Author

Thanks for the PR!

Just cleaning up after myself. :D

I'm also fine with submitting this as-is and handling separate PRs for any future issues.

While I have nothing too much against it, I feel like a single PR covering all the little mistakes made is better than a bunch of tiny ones. So I was thinking about letting this stay for a week or two (or even a month) and give us time to notice and report everything we find wrong.


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


Comments from Reviewable

@Valloric
Copy link
Member

While I have nothing too much against it, I feel like a single PR covering all the little mistakes made is better than a bunch of tiny ones. So I was thinking about letting this stay for a week or two (or even a month) and give us time to notice and report everything we find wrong.

Whatever floats your boat. :)

@vheon
Copy link
Contributor

vheon commented Mar 31, 2017

So I was thinking about letting this stay for a week or two (or even a month) and give us time to notice and report everything we find wrong.

I'm not sure what values would add to leave this open in "hope" to find something else.

:lgtm:

@bstaletic
Copy link
Collaborator Author

@vheon The only reason behind me thinking like that is the oblitum's comment stating he had noticed "some weird additions" (i.e. plural), so I thought he might have something to add to this.

But like I said, that's just an idea and I'm not that strongly against meging this as is.

@oblitum
Copy link
Contributor

oblitum commented Mar 31, 2017

I don't recall anything anymore that was actual wrong code. I didn't review all of it, just brushed over when merging in my repo and noticed several refactoring changes whose rationale I had no idea for, one of those happened to be caught in my radar while I was trying to make sense of it.

@bstaletic
Copy link
Collaborator Author

@oblitum I took a better look at the #701 changes before submitting this PR. Everything else seems fine to me. There are things like #include <cstdlib> that seem out of place, but there was a real need for those.

Feel free to ask about anything that seems odd or out of place and I'll be glad to address all of your questions.

@vheon @Valloric I guess this can be merged.

@vheon
Copy link
Contributor

vheon commented Mar 31, 2017

@homu r=Valloric

@homu
Copy link
Contributor

homu commented Mar 31, 2017

📌 Commit 824a36c has been approved by Valloric

@homu
Copy link
Contributor

homu commented Mar 31, 2017

⚡ Test exempted - status

@homu homu merged commit 824a36c into ycm-core:master Mar 31, 2017
homu added a commit that referenced this pull request Mar 31, 2017
Remove IdentifierEndsWith() declaration

Just like @oblitum  pointed out in [#701(comment)](#701 (comment)), in `CompletionData.cpp` the `IdentifierEndsWith()` function is called only from function `RemoveTrailingParens()` and both functions are in unnamed namespace, so neither should have declaration in `CompletionData.h`.

@oblitum, in his comment, mentions that he had noticed more odd changes in that PR. While this commit fixes the change he explicitely mentioned I would like to wait and see if he has to add anything else.

<!-- 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/732)
<!-- Reviewable:end -->
@bstaletic bstaletic deleted the oblitum_comment_cleanup branch March 31, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants