-
Notifications
You must be signed in to change notification settings - Fork 580
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
XMLBear: Process error correctly #255
Conversation
cancelling non circle builds here as this needs to be rebased later anyway |
repeat: the builds are not failed :) all is well |
return self._process_corrected(self.stdout_output, filename, file) | ||
if self.stderr_output: # pragma: no cover | ||
if self.stderr_output: | ||
self.use_stderr = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just define it outside, like gives corrected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_stderr isnt really needed. It is used only in lint()
and wont be used after this line changes it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbdealiJK use_stderr is needed because if not set to True it will print logs also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. you're right. Sorry - I got confused.
@sils1297 it cannot be moved outside.
unack f7fa889 |
if self.stdout_output: # only yield Result if stdout is not empty | ||
return self._process_corrected(self.stdout_output, filename, file) | ||
if self.stderr_output: # pragma: no cover | ||
self.use_stderr = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be moved to class level
return self._process_issues(self.stderr_output, filename) | ||
if self.stdout_output: | ||
return self._process_corrected(self.stdout_output, filename, file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you switch those two and why do you remove the comment in line 27?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sils1297 I switched it because if there is an error in stderr then return no need to check stdout else check if something needs to be corrected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a bug that you're trying to fix, please make a new test to test this.
Either way it would be good to add a comment here stating this so that someone else doesnt change it later by mistake
Note: You can use elif
instead of if
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbdealiJK Tests are already there which covers everything.
What more tests you want me to add ?
3ffd810
to
8e390b1
Compare
@SanketDG @AbdealiJK Any particular reason why coverage is not 100%. |
f71a897
to
f0425f0
Compare
self.use_stderr = True | ||
if self.stdout_output: # only yield Result if stdout is not empty | ||
return collections.ChainMap( | ||
self._process_issues(self.stderr_output, filename), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbdealiJK What's the issue in indentation ?
Isn't it 4 spaces PEP8 doesn't give me any error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be 4 spaces from the beginning of "return"
On Fri, Mar 18, 2016 at 2:53 PM, vivek425ster [email protected]
wrote:
In bears/xml/XMLBear.py
#255 (comment)
:@@ -24,10 +26,16 @@ class XMLBear(LocalBear, Lint):
gives_corrected = Truedef process_output(self, output, filename, file):
if self.stdout_output: # only yield Result if stdout is not empty
return self._process_corrected(self.stdout_output, filename, file)
if self.stderr_output: # pragma: no cover
if self.stderr_output:
self.use_stderr = True
if self.stdout_output: # only yield Result if stdout is not empty
return collections.ChainMap(
self._process_issues(self.stderr_output, filename),
@AbdealiJK https://github.com/AbdealiJK What's the issue in indentation
?
Isn't it 4 spaces PEP8 doesn't give me any error.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/coala-analyzer/coala-bears/pull/255/files/f0425f051fba52543fd2984750a8277b427e655a#r56629795
@SanketDG @AbdealiJK Why isn't this elif getting covered ? |
if self.stderr_output: # pragma: no cover | ||
return self._process_issues(self.stderr_output, filename) | ||
if self.stderr_output: | ||
self.use_stderr = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is needed, why do you keep this here?
The usage of use_stderr
is determined in the Lint.lint()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SanketDG It needs to be added because if it is not there then logs will also be printed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why not just use_stderr=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SanketDG if i do use_stderr = True
without checking whether the stderr_output is there or not it will set results_output
to stderr_output even when there are no any errors in stderr.
https://github.com/coala-analyzer/coala/blob/master/coalib/bearlib/abstractions/Lint.py#L98
unack 6d6e9e8 |
f3ccbe4
to
589bb64
Compare
@SanketDG @AbdealiJK @sils1297 Any idea why elif is not getting covered .From what i know https://github.com/coala-analyzer/coala-bears/blob/master/tests/xml/XMLBearTest.py#L11 this test should cover the elif case . |
seems to be covered now? |
@sils1297 Yeah i got the problem it was because if both stdout and stderr were empty nothing was being done. i.e. no else statement |
@@ -1,4 +1,3 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the stdout (which gives corrected output) adds it to the corrected one.
Hence, for coverage of Line 36.
But I havent followed this very closely. So, would be good to let @vivek425ster confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Adrianzatreanu yes it is so that corrections are added as well.
For coverage of Line 34
cf51a65 needs work |
Errors were shown as logs instead of errors. Corrected the processing of output to show the error messages. Fixes coala#251
ack 06b5761 |
ack 06b5761 |
@rultor merge |
Errors were shown as logs instead of errors. Corrected the processing of
output to show the error messages.
Fixes #251