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.

Sign up for GitHub

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

Jump to bottom

[READY] Pass ycmd logging level to JediHTTP #513

Merged
merged 1 commit into from
Jun 3, 2016
Merged

[READY] Pass ycmd logging level to JediHTTP #513

merged 1 commit into from
Jun 3, 2016

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Jun 2, 2016 •
edited by Valloric
Loading

Now that JediHTTP has an option to set its logging level, we can pass ycmd logging level to it.

This change is Reviewable

@vheon
Copy link
Contributor

vheon commented Jun 2, 2016

:lgtm:

Previously, micbou wrote…

[READY] Pass ycmd logging level to JediHTTP

Now that JediHTTP has an option to set its logging level, we can pass ycmd logging level to it.

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

Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented Jun 2, 2016

Tests are failing because no log level is set during the tests. I am looking into it.

Previously, vheon (Andrea Cedraro) wrote…

:lgtm:

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

Comments from Reviewable

@micbou micbou changed the title [READY] Pass ycmd logging level to JediHTTP [WIP] Pass ycmd logging level to JediHTTP Jun 2, 2016
@micbou micbou force-pushed the python-completer-log-level branch from 1c8f514 to bc633fb Compare June 2, 2016 15:47
@micbou micbou changed the title [WIP] Pass ycmd logging level to JediHTTP [READY] Pass ycmd logging level to JediHTTP Jun 2, 2016
@micbou
Copy link
Collaborator Author

micbou commented Jun 2, 2016

This should work now.

Previously, micbou wrote…

Tests are failing because no log level is set during the tests. I am looking into it.

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

Comments from Reviewable

@micbou micbou force-pushed the python-completer-log-level branch from bc633fb to ce3f8d9 Compare June 2, 2016 15:53
@vheon
Copy link
Contributor

vheon commented Jun 2, 2016

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

ycmd/completers/python/jedi_completer.py, line 164 [r3] (raw file):

        # Tests are run with the NOTSET logging level but JediHTTP only accept
        # the predefined levels above.
        log_level = max( self._logger.getEffectiveLevel(), logging.DEBUG )

wouldn't be easier to add notset as a valid option to JediHTTP logging?

Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented Jun 2, 2016

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

ycmd/completers/python/jedi_completer.py, line 164 [r3] (raw file):
Yes but I don't think we are supposed to use NOTSET. From the logging sources:

There is a pseudo-level, NOTSET, which is only really there as a lower limit for user-defined levels.

Also, notset as an option for the --log parameter sounds strange.

Comments from Reviewable

@vheon
Copy link
Contributor

vheon commented Jun 2, 2016

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

ycmd/completers/python/jedi_completer.py, line 164 [r3] (raw file):

Previously, micbou wrote…

Yes but I don't think we are supposed to use NOTSET. From the logging sources:

There is a pseudo-level, NOTSET, which is only really there as a lower limit for user-defined levels.

Also, notset as an option for the --log parameter sounds strange.

Fair enough 😃 but I don't like this `max` here in the middle of the function :/ could we put this and the `logging.getLevelName( log_level ).lower()` into a private method like `_GetLoggingLevel`?

Comments from Reviewable

@micbou micbou force-pushed the python-completer-log-level branch from ce3f8d9 to fb43ead Compare June 2, 2016 18:09
@micbou
Copy link
Collaborator Author

micbou commented Jun 2, 2016

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

ycmd/completers/python/jedi_completer.py, line 164 [r3] (raw file):

Previously, vheon (Andrea Cedraro) wrote…

Fair enough 😃 but I don't like this max here in the middle of the function :/ could we put this and the logging.getLevelName( log_level ).lower() into a private method like _GetLoggingLevel?

Of course.

Comments from Reviewable

@micbou micbou force-pushed the python-completer-log-level branch 2 times, most recently from f908808 to 836ebbd Compare June 2, 2016 18:15
@vheon
Copy link
Contributor

vheon commented Jun 2, 2016

:lgtm: 👍

Previously, micbou wrote…

This should work now.

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

@micbou micbou force-pushed the python-completer-log-level branch from 836ebbd to cfdb56e Compare June 2, 2016 20:57
@Valloric
Copy link
Member

Valloric commented Jun 2, 2016

Nice!

@homu r+

Previously, vheon (Andrea Cedraro) wrote…

:lgtm: 👍

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

Comments from Reviewable

@homu
Copy link
Contributor

homu commented Jun 2, 2016

📌 Commit cfdb56e has been approved by Valloric

homu added a commit that referenced this pull request Jun 2, 2016
@homu
Auto merge of #513 - micbou:python-completer-log-level, r=Valloric
9ac6e36 
[READY] Pass ycmd logging level to JediHTTP

Now that [`JediHTTP` has an option to set its logging level](vheon/JediHTTP#15), we can pass `ycmd` logging level to it.

<!-- 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/513)
<!-- Reviewable:end -->
@homu
Copy link
Contributor

homu commented Jun 2, 2016

⌛ Testing commit cfdb56e with merge 9ac6e36...

@homu
Copy link
Contributor

homu commented Jun 3, 2016

💔 Test failed - status

@vheon
Copy link
Contributor

vheon commented Jun 3, 2016

@homu retry

@homu
Copy link
Contributor

homu commented Jun 3, 2016

⌛ Testing commit cfdb56e with merge 47304e1...

homu added a commit that referenced this pull request Jun 3, 2016
@homu
Auto merge of #513 - micbou:python-completer-log-level, r=Valloric
47304e1 
[READY] Pass ycmd logging level to JediHTTP

Now that [`JediHTTP` has an option to set its logging level](vheon/JediHTTP#15), we can pass `ycmd` logging level to it.

<!-- 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/513)
<!-- Reviewable:end -->
@homu
Copy link
Contributor

homu commented Jun 3, 2016

☀️ Test successful - status

@homu homu merged commit cfdb56e into ycm-core:master Jun 3, 2016
@homu homu mentioned this pull request Jun 3, 2016
Closed
@micbou micbou deleted the python-completer-log-level branch June 4, 2016 08:36
@coveralls
Copy link

Coverage Status

Coverage decreased (-36.7%) to 48.281% when pulling cfdb56e on micbou:python-completer-log-level into 65f1987 on Valloric:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@micbou @vheon @Valloric @homu @coveralls