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] Recognise ~/.tern-config in Tern completer #280

Merged
merged 4 commits into from
Dec 22, 2015

Conversation

puremourning
Copy link
Member

Fixes #277

We simply suppress the warning if we detect a .tern-project or a user's ~/.tern-config.

Review on Reviewable

@puremourning
Copy link
Member Author

OK so coveralls shamed me into writing a test. We already require that there is no user config, so we might as well abuse that.

@puremourning puremourning changed the title [READY] Recognise ~/.tern-config in Tern completer Recognise ~/.tern-config in Tern completer Dec 20, 2015
@puremourning puremourning changed the title Recognise ~/.tern-config in Tern completer [READY] Recognise ~/.tern-config in Tern completer Dec 20, 2015
@vheon
Copy link
Contributor

vheon commented Dec 20, 2015

This :lgtm: 👍


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


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

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


ycmd/tests/javascript/event_notification_test.py, line 40 [r1] (raw file):
Doesn't this mean that a ~/.tern-config file will be overwritten if a developer runs the ycmd test suite on their machine? They might have such a file because they use Tern.js.


ycmd/tests/javascript/event_notification_test.py, line 86 [r1] (raw file):
I see, so you check for it first... still, this isn't ideal.

How about this: instead of calling os.path.expanduser( '~/.tern-config' ) directly in the code, create a wrapper PathToGlobalTernConfig function that calls that and then mock out that function in your test.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

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


ycmd/tests/javascript/event_notification_test.py, line 86 [r1] (raw file):
Or better yet, have a GlobalTernConfigExists function that returns a bool, and mock that out in tests.

In general, I've found that mocking out filesystem access for tests leads to more maintainable test code.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

I have applied the mocking approach for the tests.


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


ycmd/tests/javascript/event_notification_test.py, line 40 [r1] (raw file):
Yeah, this is unfortunate. It was the only way I could think of to test the actual code without invasive patching.


ycmd/tests/javascript/event_notification_test.py, line 86 [r1] (raw file):
Good point. It still leads to the same amount of uncoverable code, but testing the logic is more useful than not.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Nice!

@homu r+


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


Comments from the review on Reviewable.io

@homu
Copy link
Contributor

homu commented Dec 22, 2015

📌 Commit 5134865 has been approved by Valloric

@homu
Copy link
Contributor

homu commented Dec 22, 2015

⚡ Test exempted - status

@homu homu merged commit 5134865 into ycm-core:master Dec 22, 2015
homu added a commit that referenced this pull request Dec 22, 2015
[READY] Recognise ~/.tern-config in Tern completer

Fixes #277

We simply suppress the warning if we detect a `.tern-project` or a user's `~/.tern-config`.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/280)
<!-- Reviewable:end -->
@puremourning puremourning deleted the tern-global-config branch November 1, 2016 20:43
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