StandardBear: Add fixing capabilities #1984

Alexander-N · 2017-08-07T13:36:19Z

The corrected code is retrieved by piping to JSStandard. If there
several issues in a single line, the messages are displayed together to
make it easy to understand the proposed fix.

Closes #1952

gitmate-bot · 2017-08-07T13:37:20Z

tests/js/JSStandardBearTest.py

@@ -68,7 +71,7 @@
 }
 """

-JSStandardBearTest = verify_local_bear(JSStandardBear,
+JSStandardBearTestFiles = verify_local_bear(JSStandardBear,
                                       valid_files=(good_file,),
                                       invalid_files=(bad_file_indent,


E128 continuation line under-indented for visual indent'

PycodestyleBear (E128), severity NORMAL, section autopep8.

gitmate-bot · 2017-08-07T13:37:20Z

tests/js/JSStandardBearTest.py

@@ -68,7 +71,7 @@
 }
 """

-JSStandardBearTest = verify_local_bear(JSStandardBear,
+JSStandardBearTestFiles = verify_local_bear(JSStandardBear,
                                       valid_files=(good_file,),


E128 continuation line under-indented for visual indent'

PycodestyleBear (E128), severity NORMAL, section autopep8.

gitmate-bot · 2017-08-07T13:37:21Z

tests/js/JSStandardBearTest.py

@@ -68,7 +71,7 @@
 }
 """

-JSStandardBearTest = verify_local_bear(JSStandardBear,
+JSStandardBearTestFiles = verify_local_bear(JSStandardBear,
                                       valid_files=(good_file,),


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/js/JSStandardBearTest.py +++ b/tests/js/JSStandardBearTest.py @@ -72,14 +72,14 @@ """ JSStandardBearTestFiles = verify_local_bear(JSStandardBear, - valid_files=(good_file,), - invalid_files=(bad_file_indent, - bad_file_quote, - bad_file_semicolon, - bad_file_infix, - bad_file_undef, - bad_file_ifelse, - bad_file_func_name,)) + valid_files=(good_file,), + invalid_files=(bad_file_indent, + bad_file_quote, + bad_file_semicolon, + bad_file_infix, + bad_file_undef, + bad_file_ifelse, + bad_file_func_name,)) class JSStandardBearTest(LocalBearTestHelper):

gitmate-bot · 2017-08-07T13:47:06Z

tests/js/JSStandardBearTest.py

+                                                           bad_file_infix,
+                                                           bad_file_undef,
+                                                           bad_file_ifelse,
+                                                           bad_file_func_name,))


E501 line too long (80 > 79 characters)'

PycodestyleBear (E501), severity NORMAL, section autopep8.

gitmate-bot · 2017-08-07T13:47:07Z

tests/js/JSStandardBearTest.py

+                                                           bad_file_infix,
+                                                           bad_file_undef,
+                                                           bad_file_ifelse,
+                                                           bad_file_func_name,))


Line is longer than allowed. (80 > 79)

LineLengthBear, severity NORMAL, section linelength.

Alexander-N · 2017-09-04T12:12:34Z

This PR is ready for review. Anybody?

sils · 2017-10-16T16:22:21Z

hey @Alexander-N can you add a testcase that actually tests that the patch is correct? Like if there's multiple results from standard does it properly figure out which part of the diff belongs to the right result? That's a thing that I'd definitely want to see in the tests

gitmate-bot · 2017-10-22T17:09:50Z

Comment on 9c7d939.

Body of HEAD commit contains too long lines. Commit body lines should not exceed 72 characters.

Origin: GitCommitBear, Section: commit.

Alexander-N · 2017-10-23T22:45:59Z

Ok, done.

Makman2 · 2017-12-18T15:23:59Z

Sorry it took so long and we haven't done a review :/ We are a bit out of manpower in general for such things, so I hope you can understand :/

If you still want to give it a try, do you mind to resolve the conflicts and push again? I'm gonna review this then 👍

Get the corrected code by piping to "standard". If there are several issues in a single line, the messages are displayed together to to make it possible to understand the fix. Closes coala#1952

Alexander-N · 2017-12-19T23:58:43Z

No Problem, lets do this :-)

Conflicts resolved.

Makman2 · 2017-12-25T18:51:23Z

bears/js/JSStandardBear.py

+        """
+        p = Popen(
+            ('standard', '--stdin', '--fix'),
+            stdin=PIPE, stdout=PIPE, stderr=PIPE)


do we have to execute standard twice to get the corrected code?

If we want the messages and the corrected code I see no other way.

Makman2 · 2017-12-25T18:52:48Z

bears/js/JSStandardBear.py

+            ('standard', '--stdin', '--fix'),
+            stdin=PIPE, stdout=PIPE, stderr=PIPE)
+        p.stdin.write(bytes(''.join(old_code), 'UTF-8'))
+        out, err = p.communicate()


we have to be careful, this can deadlock if the buffers get full (and because we might operate with larger files, this can happen). Are there alternatives that take care of this?

Maybe temporary files instead? I checked some bigger files, standard itself is failing if ~500kB files are piped. I should also check the return code at this point.

There should be special functions somewhere to call a program with stdin and retrieve stdout and stderr without deadlocking.

I should also check the return code at this point.

Oh yeah good point 👍

Makman2 · 2017-12-25T18:54:02Z

tests/js/JSStandardBearTest.py

 from bears.js.JSStandardBear import JSStandardBear

-


unnecessary change ;)

Makman2 · 2017-12-25T19:09:27Z

bears/js/JSStandardBear.py

+            diff = None
+            if corrected_code:
+                diff = Diff(file)
+                diff.modify_line(line_number, corrected_code[line_number-1])


can a patch span multiple lines?

Not that I'm aware of.

But looking at this again, it's not robust at all. If standards "--fix" would remove a line and add one at some other place, the patches could be wrong.

Alexander-N · 2017-12-26T17:15:40Z

I'm doubtful if it is worth moving on with this. What do you think? Are there any alternatives I'm not seeing?

Makman2 · 2017-12-27T23:47:44Z

I'm doubtful if it is worth moving on with this. What do you think? Are there any alternatives I'm not seeing?

Hm this worries me a bit:

But looking at this again, it's not robust at all. If standards "--fix" would remove a line and add one at some other place, the patches could be wrong.

I don't know the tools behaviour, so I can't say what edge cases might occur.

What's actually best is if the tool itself would print out a format that assigns patches to issue messages, something like

L3C3 Invalid identifier
-- @unified-diff patch attached --
@@ ...
<unified diff goes here>
----------
L4C4 Invalid xyz
...

Usually tools don't have something like this sadly (could be actually worth to define one - or maybe a few - such formats in a coala cEP). This would be the optimal solution, having an upstream contribution doing this (then we also don't have to invoke standard twice ;D). But this is probably very time-consuming^^

Alexander-N · 2018-01-06T15:40:32Z

Yes I agree. Close, since the approach seems problematic.

gitmate-bot added process/pending review size/L labels Aug 7, 2017

gitmate-bot reviewed Aug 7, 2017

View reviewed changes

Alexander-N force-pushed the jsstandardbear-fix branch from 25f29f0 to 88ae03a Compare August 7, 2017 13:46

gitmate-bot reviewed Aug 7, 2017

View reviewed changes

Alexander-N force-pushed the jsstandardbear-fix branch 8 times, most recently from b63b6ad to 00390c5 Compare August 8, 2017 17:11

gitmate-bot added the status/STALE label Aug 31, 2017

gitmate-bot removed the status/STALE label Oct 16, 2017

Alexander-N force-pushed the jsstandardbear-fix branch from 00390c5 to 9c7d939 Compare October 22, 2017 17:09

gitmate-bot added size/S difficulty/low status/STALE and removed size/L labels Oct 22, 2017

gitmate-bot assigned Alexander-N Oct 22, 2017

gitmate-bot removed the status/STALE label Oct 22, 2017

Alexander-N force-pushed the jsstandardbear-fix branch from 9c7d939 to 5c942ef Compare October 22, 2017 17:13

gitmate-bot added the status/STALE label Oct 22, 2017

Alexander-N force-pushed the jsstandardbear-fix branch from 5c942ef to da82769 Compare October 23, 2017 22:15

gitmate-bot added status/STALE and removed status/STALE labels Oct 23, 2017

gitmate-bot removed the status/STALE label Oct 23, 2017

gitmate-bot added the status/STALE label Oct 31, 2017

gitmate-bot removed the status/STALE label Dec 18, 2017

Alexander-N force-pushed the jsstandardbear-fix branch 3 times, most recently from acc69bf to ef60ad5 Compare December 19, 2017 21:33

JSStandardBear: Add fixing capabilities

d66d7d3

Get the corrected code by piping to "standard". If there are several issues in a single line, the messages are displayed together to to make it possible to understand the fix. Closes coala#1952

Alexander-N force-pushed the jsstandardbear-fix branch from ef60ad5 to d66d7d3 Compare December 19, 2017 23:21

Makman2 reviewed Dec 25, 2017

View reviewed changes

tests/js/JSStandardBearTest.py

from bears.js.JSStandardBear import JSStandardBear

Copy link

Member

Makman2 Dec 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary change ;)

Makman2 reviewed Dec 25, 2017

View reviewed changes

Alexander-N closed this Jan 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StandardBear: Add fixing capabilities #1984

StandardBear: Add fixing capabilities #1984

Alexander-N commented Aug 7, 2017

gitmate-bot Aug 7, 2017

gitmate-bot Aug 7, 2017

gitmate-bot Aug 7, 2017

gitmate-bot Aug 7, 2017

gitmate-bot Aug 7, 2017

Alexander-N commented Sep 4, 2017

sils commented Oct 16, 2017

gitmate-bot commented Oct 22, 2017

Alexander-N commented Oct 23, 2017

Makman2 commented Dec 18, 2017

Alexander-N commented Dec 19, 2017

Makman2 Dec 25, 2017

Alexander-N Dec 26, 2017

Makman2 Dec 25, 2017

Alexander-N Dec 26, 2017

Makman2 Dec 27, 2017

Makman2 Dec 25, 2017

Makman2 Dec 25, 2017

Alexander-N Dec 26, 2017 •

edited

Loading

Alexander-N commented Dec 26, 2017

Makman2 commented Dec 27, 2017

Alexander-N commented Jan 6, 2018

StandardBear: Add fixing capabilities #1984

StandardBear: Add fixing capabilities #1984

Conversation

Alexander-N commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexander-N commented Sep 4, 2017

sils commented Oct 16, 2017

gitmate-bot commented Oct 22, 2017

Alexander-N commented Oct 23, 2017

Makman2 commented Dec 18, 2017

Alexander-N commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexander-N Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Alexander-N commented Dec 26, 2017

Makman2 commented Dec 27, 2017

Alexander-N commented Jan 6, 2018

Alexander-N Dec 26, 2017 •

edited

Loading