Remove repeated definitions of format_lines() #5215

nityeshaga · 2018-02-27T17:37:30Z

Replaces the definitions of format_lines() in files- coala_main.py
and ShowAppliedPatchAction.py with import statements that import the
function from ConsoleInteraction.py.

Closes #5205

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

pick up any PR at https://coala.io/review
review it (check https://coala.io/reviewing for more info)
if you are sure that it needs work, use cobot mark wip <URL> to get it out
of the review queue.

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

nityeshaga · 2018-02-27T17:39:26Z

coalib/results/result_actions/ShowAppliedPatchesAction.py

@@ -32,6 +26,7 @@ def apply(self,
        FILE_DIFF_DICT_INDEX = 2
        SECTION_INDEX = 3

+        from coalib.output.ConsoleInteraction import format_lines


The import statement had to be placed here instead of the usual - top of the file - because coala was encountering an error with circularly dependent import.

Then put the function in a different module which doesnt cause a circular dependency

There's a circular dependency problem with ShowAppliedPatches.py regardless of whether I put the function in ConsoleInteraction.py or coala_main.py. Now, the function format_lines() is used only in these 3 modules-- ShowAppliedPatches.py, ConsoleInteraction.py and coala_main.py.

Therefore, I IMHO I think that we need to put the function definition in one of these files only. Also, I don't think that it should be inside ShowAppliedPatches.py.

Hence, the confusion. 😕

Please suggest a fix to it @jayvdb

It is your task to find the solution.
I already know it.
And telling it to you doesn't help you learn.

Naveenaidu

@nityeshaga LGTM 😄 . 👍 for mentioning the comment about the import statement.

jayvdb · 2018-03-10T06:31:59Z

coalib/results/result_actions/ShowAppliedPatchesAction.py

@@ -32,6 +26,7 @@ def apply(self,
        FILE_DIFF_DICT_INDEX = 2
        SECTION_INDEX = 3

+        from coalib.output.ConsoleInteraction import format_lines


Then put the function in a different module which doesnt cause a circular dependency

nityeshaga · 2018-04-10T14:47:50Z

I think that the Interactions.py module is the right home for the format_lines() function. ;-) So, I have moved its definition over there.

I have also moved the definitions of highlight_text() and color_letter() in that file too as IMHO they would kind of fall in the same category as format_lines().

@jayvdb What do you think?

jayvdb · 2018-04-10T16:56:44Z

get a green build. then we talk

gitmate-bot · 2018-04-11T05:36:28Z

tests/output/InteractionsTest.py

@@ -2,7 +2,9 @@

 from pyprint.NullPrinter import NullPrinter

-from coalib.output.Interactions import fail_acquire_settings
+from coala_utils.ContextManagers import retrieve_stdout
+from coalib.output.Interactions import (fail_acquire_settings, 


Line contains following spacing inconsistencies:

Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py +++ b/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py @@ -3,7 +3,7 @@ from pyprint.NullPrinter import NullPrinter from coala_utils.ContextManagers import retrieve_stdout -from coalib.output.Interactions import (fail_acquire_settings, +from coalib.output.Interactions import (fail_acquire_settings, highlight_text, color_letter, format_lines) from coalib.output.printers.LogPrinter import LogPrinter

gitmate-bot · 2018-04-11T05:36:29Z

tests/output/InteractionsTest.py

@@ -2,7 +2,9 @@

 from pyprint.NullPrinter import NullPrinter

-from coalib.output.Interactions import fail_acquire_settings
+from coala_utils.ContextManagers import retrieve_stdout


This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py +++ b/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py @@ -2,7 +2,6 @@ from pyprint.NullPrinter import NullPrinter -from coala_utils.ContextManagers import retrieve_stdout from coalib.output.Interactions import (fail_acquire_settings, highlight_text, color_letter, format_lines) from coalib.output.printers.LogPrinter import LogPrinter

gitmate-bot · 2018-04-11T05:36:30Z

tests/output/InteractionsTest.py

@@ -2,7 +2,9 @@

 from pyprint.NullPrinter import NullPrinter

-from coalib.output.Interactions import fail_acquire_settings
+from coala_utils.ContextManagers import retrieve_stdout
+from coalib.output.Interactions import (fail_acquire_settings, 


The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py +++ b/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py @@ -3,8 +3,8 @@ from pyprint.NullPrinter import NullPrinter from coala_utils.ContextManagers import retrieve_stdout -from coalib.output.Interactions import (fail_acquire_settings, - highlight_text, color_letter, format_lines) +from coalib.output.Interactions import (fail_acquire_settings, + highlight_text, color_letter, format_lines) from coalib.output.printers.LogPrinter import LogPrinter

gitmate-bot · 2018-04-11T05:36:31Z

tests/output/InteractionsTest.py

+    def test_format_lines(self):
+        prompt_msg = 'Enter a number: '
+        prompt_msg_formatted = '[   ] Enter a number: '
+        self.assertEqual(format_lines(prompt_msg, symbol='['), prompt_msg_formatted)


The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py +++ b/tmp/tmp4c82t5xa/tests/output/InteractionsTest.py @@ -22,4 +22,5 @@ def test_format_lines(self): prompt_msg = 'Enter a number: ' prompt_msg_formatted = '[ ] Enter a number: ' - self.assertEqual(format_lines(prompt_msg, symbol='['), prompt_msg_formatted) + self.assertEqual(format_lines( + prompt_msg, symbol='['), prompt_msg_formatted)

gitmate-bot · 2018-04-11T05:36:32Z

tests/output/InteractionsTest.py

@@ -2,7 +2,9 @@

 from pyprint.NullPrinter import NullPrinter

-from coalib.output.Interactions import fail_acquire_settings
+from coala_utils.ContextManagers import retrieve_stdout
+from coalib.output.Interactions import (fail_acquire_settings, 


W291 trailing whitespace

Origin: PycodestyleBear (W291), Section: autopep8.

gitmate-bot · 2018-04-11T05:36:33Z

tests/output/InteractionsTest.py

-from coalib.output.Interactions import fail_acquire_settings
+from coala_utils.ContextManagers import retrieve_stdout
+from coalib.output.Interactions import (fail_acquire_settings, 
+        highlight_text, color_letter, format_lines)


E128 continuation line under-indented for visual indent

Origin: PycodestyleBear (E128), Section: autopep8.

gitmate-bot · 2018-04-11T14:06:31Z

tests/output/InteractionsTest.py

+
+            color_letter(self.console_printer, message)
+
+            self.assertEqual(stdout.getvalue().strip(), first_part + 


Line contains following spacing inconsistencies:

Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py +++ b/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py @@ -44,7 +44,7 @@ color_letter(self.console_printer, message) - self.assertEqual(stdout.getvalue().strip(), first_part + + self.assertEqual(stdout.getvalue().strip(), first_part + colored(letter, color='blue') + second_part)

gitmate-bot · 2018-04-11T14:06:32Z

tests/output/InteractionsTest.py

+            color_letter(self.console_printer, message)
+
+            self.assertEqual(stdout.getvalue().strip(), first_part + 
+                                                        colored(letter, 


Line contains following spacing inconsistencies:

Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py +++ b/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py @@ -45,7 +45,7 @@ color_letter(self.console_printer, message) self.assertEqual(stdout.getvalue().strip(), first_part + - colored(letter, + colored(letter, color='blue') + second_part)

gitmate-bot · 2018-04-11T14:06:33Z

tests/output/InteractionsTest.py

+        text = 'Text to be highlighted'
+        #formatter = TerminalTrueColorFormatter(style=NoColorStyle)
+        #highlighted_text = highlight(text, TextLexer(), formatter)[:-1]
+        self.assertEqual(highlight_text(self.no_color, text, NoColorStyle), 


Line contains following spacing inconsistencies:

Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py +++ b/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py @@ -53,5 +53,5 @@ text = 'Text to be highlighted' #formatter = TerminalTrueColorFormatter(style=NoColorStyle) #highlighted_text = highlight(text, TextLexer(), formatter)[:-1] - self.assertEqual(highlight_text(self.no_color, text, NoColorStyle), + self.assertEqual(highlight_text(self.no_color, text, NoColorStyle), text)

gitmate-bot · 2018-04-11T14:06:34Z

tests/output/InteractionsTest.py

+        #formatter = TerminalTrueColorFormatter(style=NoColorStyle)
+        #highlighted_text = highlight(text, TextLexer(), formatter)[:-1]
+        self.assertEqual(highlight_text(self.no_color, text, NoColorStyle), 
+                            text) 


Line contains following spacing inconsistencies:

Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py +++ b/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py @@ -54,4 +54,4 @@ #formatter = TerminalTrueColorFormatter(style=NoColorStyle) #highlighted_text = highlight(text, TextLexer(), formatter)[:-1] self.assertEqual(highlight_text(self.no_color, text, NoColorStyle), - text) + text)

gitmate-bot · 2018-04-11T14:06:35Z

tests/output/InteractionsTest.py

@@ -1,13 +1,25 @@
 import unittest

 from pyprint.NullPrinter import NullPrinter
+from pyprint.ConsolePrinter import ConsolePrinter
+from termcolor import colored
+from pygments.formatters import TerminalTrueColorFormatter


This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py +++ b/tmp/tmpz0nm9lb2/tests/output/InteractionsTest.py @@ -3,7 +3,6 @@ from pyprint.NullPrinter import NullPrinter from pyprint.ConsolePrinter import ConsolePrinter from termcolor import colored -from pygments.formatters import TerminalTrueColorFormatter from coala_utils.ContextManagers import retrieve_stdout from coalib.output.Interactions import (fail_acquire_settings,

jayvdb · 2018-04-12T00:52:50Z

Please stop pushing until you have a version which passes the tests on your local machine.

nityeshaga · 2018-04-12T03:01:02Z

Extremely sorry for the mess. 😅

Please stop pushing until you have a version which passes the tests on your local machine.

I'll keep this in mind

gitmate-bot · 2018-04-12T18:42:30Z

tests/output/InteractionsTest.py

@@ -1,9 +1,18 @@
 import unittest

 from pyprint.NullPrinter import NullPrinter
+from pyprint.ConsolePrinter import ConsolePrinter
+from termcolor import colored
+from pygments.formatters import TerminalTrueColorFormatter


This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpx0fit6qr/tests/output/InteractionsTest.py +++ b/tmp/tmpx0fit6qr/tests/output/InteractionsTest.py @@ -3,7 +3,6 @@ from pyprint.NullPrinter import NullPrinter from pyprint.ConsolePrinter import ConsolePrinter from termcolor import colored -from pygments.formatters import TerminalTrueColorFormatter from coala_utils.ContextManagers import retrieve_stdout from coalib.output.Interactions import (fail_acquire_settings,

jayvdb · 2018-04-14T10:59:34Z

coalib/output/Interactions.py

@@ -28,3 +45,35 @@ def fail_acquire_settings(log_printer, settings_names_dict):

        logging.error(msg)
        raise AssertionError(msg)
+
+
+def highlight_text(no_color, text, style, lexer=TextLexer()):


so now instead of format_lines being duplicated, coalib/output/ConsoleInteraction.py:highlight_text is being duplicated here.

How is this better?

Sorry, I've fixed it now.
No duplicates of highlight_text() anymore :)

So, why the Interactions module?

This module now needs a docstring that would explain why fail_acquire_settings and format_lines are grouped together.

Would it be better if I created a new module Formatting.py and moved format_lines(), highlight_text() and color_letter() over there?

Removes the different definitions of format_lines() in files- ConsoleInteraction.py, coala_main.py and ShowAppliedPatchAction.py and the definitions of highlight_text() and color_letter() from ConsoleInteraction.py and moves them to Interactions.py. Closes coala#5205

jayvdb · 2018-04-15T15:24:01Z

coalib/results/result_actions/ShowAppliedPatchesAction.py

-    def sym(x): return ']' if x is '[' else x
-    return '\n'.join('{}{:>5}{} {}'.format(symbol, sym(symbol), line_nr, line)
-                     for line in lines.rstrip('\n').split('\n'))
+from coalib.output.Interactions import format_lines


why do we even need to use format_lines in this module?

And are you sure that padding of 5 is not intentional, and 4 is good enough?

And are you sure that padding of 5 is not intentional, and 4 is good enough?

In ShowAppliedPatchesAction, format_lines() is used when printing the Severity of the patch.
Here's how it looked (before this PR):
Notice the line ! ![Severity: NORMAL]

And here's how it looks now (after this PR):

So, the version of format_lines() in this PR works like it was expected to IMO.

why do we even need to use format_lines in this module?

Yes I agree, that was a bad decision on my part. 😅
I wasn't sure if creating a new file to solve a difficulty/low issue was allowed or not. Now I think it would be better if I create a new file (Formatting.py maybe?) and put format_lines(), color_letter() and highlight_text() inside it. What do you think @jayvdb ?

No you missing my point. what does format_lines do? Why is that functionality needed in ShowAppliedPatchesAction.py?

Is it the right tool for the job?

what does format_lines do?

format_lines returns a formatted version of the string passed to it as a parameter. It formats the string to make it look elegantly in coala's CLI by adding a little prefix to it.

Why is that functionality needed in ShowAppliedPatchesAction.py?

ShowAppliedPatchesAction already uses it to format the line which displays the Severity of any applied action.

So, unless you are suggesting that we change how ShowAppliedPatchesAction shows its messages, I think that format_lines is the right tool for the job.

You still have not read format_lines and its usages carefully.
It is gobbledy-gook that people have copied and pasted without thinking.

You still have not read format_lines and its usages carefully.

😅 I'm sorry, I didn't find anything else to read about it except the 4 line definition which was present in all 3 of the above mentioned files.

So, are you suggesting that we modify the definition of format_lines()? maybe make pass the amount of padding as a parameter? Or are you suggesting that we remove the function altogether?

Read those four lines very carefully. Understand exactly what happens when it is called each time.

Here's what I understand format_lines() is doing:
Each time the function is called with a string (lines), it splits that string at \ns, prefixes each split string with the symbol (passed to it as a parameter), adds line_nr with a padding of 4, adds the sym() version of symbol, and finally joins them using \n and returns this joined version.
Also, sym() is a local function inside format_lines() which is just responsible to return the closing ] in case the symbol that is passed is [. Otherwise, it returns the passed character itself.

Isn't that all? 😅

No I dont want you to tell me what it does in theory. I already know the answers.

You need to see why in practise , in some of the real world scenarios where it is used, it isnt particularly useful.

gitmate-bot added process/pending review size/XS area/CLI difficulty/low type/codestyle labels Feb 27, 2018

gitmate-bot assigned nityeshaga Feb 27, 2018

nityeshaga commented Feb 27, 2018

View reviewed changes

Naveenaidu approved these changes Mar 4, 2018

View reviewed changes

jayvdb requested changes Mar 10, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Mar 10, 2018

gitmate-bot added status/stale and removed status/stale labels Mar 18, 2018

nityeshaga force-pushed the removeRedun3 branch from 3c475b8 to 0ce555b Compare April 10, 2018 14:40

gitmate-bot added process/pending review and removed process/wip labels Apr 10, 2018

nityeshaga force-pushed the removeRedun3 branch from 0ce555b to 453d807 Compare April 10, 2018 14:45

jayvdb added process/wip and removed process/pending review labels Apr 10, 2018

nityeshaga force-pushed the removeRedun3 branch from 453d807 to 1c4b7c9 Compare April 11, 2018 05:35

gitmate-bot added process/pending review and removed process/wip labels Apr 11, 2018

gitmate-bot reviewed Apr 11, 2018

View reviewed changes

nityeshaga force-pushed the removeRedun3 branch from b52e045 to 3e8556b Compare April 11, 2018 14:11

jayvdb added process/wip and removed process/pending review labels Apr 12, 2018

nityeshaga force-pushed the removeRedun3 branch from 3e8556b to eaca416 Compare April 12, 2018 18:42

gitmate-bot added process/pending review and removed process/wip labels Apr 12, 2018

gitmate-bot reviewed Apr 12, 2018

View reviewed changes

nityeshaga force-pushed the removeRedun3 branch 3 times, most recently from baeca3e to 0d0a9fb Compare April 14, 2018 10:46

jayvdb requested changes Apr 14, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Apr 14, 2018

nityeshaga force-pushed the removeRedun3 branch from 0d0a9fb to 08dad88 Compare April 14, 2018 11:25

gitmate-bot added process/pending review and removed process/wip labels Apr 14, 2018

nityeshaga force-pushed the removeRedun3 branch from 08dad88 to 8dd4da5 Compare April 14, 2018 11:46

jayvdb requested a review from palash25 April 14, 2018 15:07

jayvdb requested changes Apr 15, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Apr 15, 2018

jayvdb mentioned this pull request Apr 19, 2018

Split ConsoleInteraction.py #2891

Closed


		color_letter(self.console_printer, message)

		self.assertEqual(stdout.getvalue().strip(), first_part +

Remove repeated definitions of format_lines() #5215

Are you sure you want to change the base?

Remove repeated definitions of format_lines() #5215

Conversation

nityeshaga commented Feb 27, 2018 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityeshaga Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naveenaidu left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityeshaga commented Apr 10, 2018

jayvdb commented Apr 10, 2018

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

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

jayvdb commented Apr 12, 2018

nityeshaga commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb Apr 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityeshaga Apr 16, 2018 • edited

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

nityeshaga Apr 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityeshaga commented Feb 27, 2018 •

edited

nityeshaga Mar 13, 2018 •

edited

Naveenaidu left a comment •

edited

jayvdb Apr 14, 2018 •

edited

nityeshaga Apr 16, 2018 •

edited

nityeshaga Apr 19, 2018 •

edited