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

Add tex identifier regex #480

Merged
merged 3 commits into from
May 8, 2016
Merged

Add tex identifier regex #480

merged 3 commits into from
May 8, 2016

Conversation

bijancn
Copy link
Contributor

@bijancn bijancn commented May 7, 2016

Commonly used latex identifiers are not covered by the default regex.
The new regex is covered by test cases similar to the ones for perl and haskell.

With this regex it is easy to reference with YCM from all figures, sections, etc. in the document. Using fig: and so on is best practice in latex but I haven't found a spec that could serve as reference.

I wasn't able to run the full test suite (see message on ycmd-users) but I ran

nosetests ycmd/tests/identifier_utils_test.py

successfully.

Ultimatively, I think the user should be able to give a custom identifier regex for different filetypes in their .vimrc (and its analogue in other editors).


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.901% when pulling f65c779 on bijancn:add-tex-identifier-regex into 8617de8 on Valloric:master.

@Valloric
Copy link
Member

Valloric commented May 7, 2016

Thanks for the PR!

:lgtm:

Ultimatively, I think the user should be able to give a custom identifier regex for different filetypes in their .vimrc (and its analogue in other editors).

This would lead to a similar situation we have today with the g:ycm_semantic_triggers option. People who know what a better trigger should be for a language fix it for themselves and never send a PR, so everyone else loses out.

Unfortunately we have to more strongly motivate people to do the right thing.


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


ycmd/identifier_utils.py, line 98 [r1] (raw file):
Shouldn't this use + instead of *? I doubt an empty string is a valid identifier. Having a test for empty string would be a good idea too.

While you're at it, could you fix the haskell one above too? It has the same issue.


Comments from Reviewable

@bijancn
Copy link
Contributor Author

bijancn commented May 7, 2016

I was just concerned that in some languages it might be a matter of convention what should be allowed as identifier. But I guess YCM can always cover the bigger set.

I changed the * to + as it makes more sense but the empty test also works with *.
It is catched by this if

def IsIdentifier( text, filetype = None ):
  if not text:
    return False

BTW shouldn't frozendict and bottle be in the test_requirements.txt? I needed them to run the test.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.901% when pulling 71884f4 on bijancn:add-tex-identifier-regex into 8617de8 on Valloric:master.

@Valloric
Copy link
Member

Valloric commented May 7, 2016

BTW shouldn't frozendict and bottle be in the test_requirements.txt? I needed them to run the test.

They're in third_party folder. I recommend using our vagrant environment for ycmd development. See DEV_SETUP.

@bijancn
Copy link
Contributor Author

bijancn commented May 8, 2016

Well I tried vagrant. On one machine I couldn't use it because in the Ubuntu 14.04 packages there is only an old version that doesn't find your machine and I have no root access there. On the other machine I think it failed due to a lack of RAM and/or disk space. Something a bit more lightweight like docker would be nice.

Anyhow, if I understand this reviewable correctly now this PR only needs two thumbs up?

@vheon
Copy link
Contributor

vheon commented May 8, 2016

there is only an old version that doesn't find your machine

I'm sorry, what do you mean? An old version of what? And which machine of ours don't you find?

@bijancn
Copy link
Contributor Author

bijancn commented May 8, 2016

Sorry for not being clear. The version of vagrant (1.4.3) in the Ubuntu 14.04 packages does not work with your Vagrantfile which demands "ubuntu/trusty64" which is to be expected if I understand Varying-Vagrant-Vagrants/VVV#354 correctly. So there is no trusty inception possible ;). And I didn't want to go through the hassle of building vagrant myself just to be able to build ycmd.

@vheon
Copy link
Contributor

vheon commented May 8, 2016

"ubuntu/trusty64"

That is the image that will be downloaded from the vagrantcloud, not the version of ubuntu required.

@bijancn
Copy link
Contributor Author

bijancn commented May 8, 2016

Yes I am aware of that. Vagrant 1.4.3 does not find trusty64 in the vagrantcloud as a lot of people are confirming in the issue I mentioned.

@vheon
Copy link
Contributor

vheon commented May 8, 2016

Yes I am aware of that

Sorry 😜

@puremourning
Copy link
Member

If you're not able to get our vagrant setup working, then there are some manual instructions.

Alternatively, if you're totally stuck, you can just enable Travis for your fork and Travis will run everything when you push a commit. Painful and slow TBH. I successfully run the tests on multiple OSes outside of our vagrant setup.

@bijancn
Copy link
Contributor Author

bijancn commented May 8, 2016

Thanks 😃. I will try manual installation next time harder.

Would still be happy to see 2 LGTMs as all tests pass.

@puremourning
Copy link
Member

:lgtm:

Thanks!

@homu r=Valloric

Previously, bijancn wrote…

Thanks 😃. I will try manual installation next time harder.

Would still be happy to see 2 LGTMs as all tests pass.


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


Comments from Reviewable

@homu
Copy link
Contributor

homu commented May 8, 2016

📌 Commit 71884f4 has been approved by Valloric

@homu
Copy link
Contributor

homu commented May 8, 2016

⌛ Testing commit 71884f4 with merge d96c633...

homu added a commit that referenced this pull request May 8, 2016
Add tex identifier regex

Commonly used latex identifiers are not covered by the default regex.
The new regex is covered by test cases similar to the ones for perl and haskell.

With this regex it is easy to reference with YCM from all figures, sections, etc. in the document. Using `fig:` and so on is best practice in latex but I haven't found a spec that could serve as reference.

I wasn't able to run the full test suite (see message on ycmd-users) but I ran
```
nosetests ycmd/tests/identifier_utils_test.py
```
successfully.

Ultimatively, I think the user should be able to give a custom identifier regex for different filetypes in their .vimrc (and its analogue in other editors).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/480)
<!-- Reviewable:end -->
@homu
Copy link
Contributor

homu commented May 8, 2016

☀️ Test successful - status

@homu homu merged commit 71884f4 into ycm-core:master May 8, 2016
@bijancn bijancn deleted the add-tex-identifier-regex branch May 8, 2016 18:55
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