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

[READY] Setting a max candidate size limit (80 chars) #370

Merged
merged 1 commit into from
Feb 17, 2016
Merged

Conversation

Valloric
Copy link
Member

This prevents issues with auto-generated strings entering the database
and screwing everything up.

Fixes ycm-core/YouCompleteMe#520

Review on Reviewable

This prevents issues with auto-generated strings entering the database
and screwing everything up.

Fixes ycm-core/YouCompleteMe#520
@vheon
Copy link
Contributor

vheon commented Feb 16, 2016

:lgtm:

I think we have a problem with Travis, looks like pypa/pip#2931 but I'm not sure 😕


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


cpp/ycm/CandidateRepository.cpp, line 40 [r1] (raw file):
I'm more than ok with this because 80 char is a reasonable default, but I'm sure that we will have requests for making this a settings.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member Author

I noticed the Travis issue and I'm looking into it. This seems relevant.

Weird, because the py3.3 run in the matrix worked before. New version of setuptools was uploaded to PyPI on 2016-02-12 (see table), so I think that might have broken us.


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


cpp/ycm/CandidateRepository.cpp, line 40 [r1] (raw file):
Oh I know we'll get people asking to make this an option, but that's not going to happen. It's only being added to prevent issues with auto-indent extraction using idents like base64-encoded images and the like. Candidates that are much too long.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Feb 16, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


cpp/ycm/CandidateRepository.cpp, line 40 [r1] (raw file):
Perfect :) So we know that we can close those issue as soon as they came up.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member Author

@micbou @puremourning Review? :)

@puremourning
Copy link
Member

I'll try and get to this tonight. :)

On 17 Feb 2016, at 17:38, Val Markovic notifications@github.com wrote:

@micbou @puremourning Review? :)


Reply to this email directly or view it on GitHub.

@puremourning
Copy link
Member

OK, yeah. this :lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member

I think homu will merge this and it will fix travis because of the previous PR, so @homu r+


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@homu
Copy link
Contributor

homu commented Feb 17, 2016

📌 Commit e6d0e49 has been approved by puremourning

@homu
Copy link
Contributor

homu commented Feb 17, 2016

⌛ Testing commit e6d0e49 with merge e93be12...

homu added a commit that referenced this pull request Feb 17, 2016
[READY] Setting a max candidate size limit (80 chars)

This prevents issues with auto-generated strings entering the database
and screwing everything up.

Fixes ycm-core/YouCompleteMe#520

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/370)
<!-- Reviewable:end -->
@homu
Copy link
Contributor

homu commented Feb 17, 2016

☀️ Test successful - status

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.

4 participants