coala-quickstart.py: Change the printing of bears#227
Open
tallicamike wants to merge 1 commit intocoala:masterfrom
Open
coala-quickstart.py: Change the printing of bears#227tallicamike wants to merge 1 commit intocoala:masterfrom
tallicamike wants to merge 1 commit intocoala:masterfrom
Conversation
Member
|
@gitmate-bot rebase |
|
Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently |
|
Automated rebase with GitMate.io was successful! 🎉 |
Travis tests have failedHey @tallicamike, 3rd Buildpytest |
ishanSrt
reviewed
May 25, 2018
| from coala_quickstart.Constants import ( | ||
| IMPORTANT_BEAR_LIST, ALL_CAPABILITIES, DEFAULT_CAPABILTIES) | ||
| from coala_quickstart.Strings import BEAR_HELP | ||
| from coala_quickstart.Strings import (PRINT_BEARS, BEAR_HELP) |
Contributor
There was a problem hiding this comment.
no need of paranthesis here
ishanSrt
reviewed
May 26, 2018
| """ | ||
| Prints the relevant bears in sections separated by language. | ||
| Prints both the usable and unusable, relevant bears | ||
| in sections separated by language. |
Contributor
There was a problem hiding this comment.
"seperated by language?"
needs a language change over here 😉
ishanSrt
reviewed
May 26, 2018
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| \nBased on the configuration options the following | ||
| bears have been identified to be unusable: |
Contributor
There was a problem hiding this comment.
an extra message on how to make these bears usable too would be good over here
Contributor
There was a problem hiding this comment.
indicating that these bears can be made usable if coala-quickstart is not run in non-interactive mode (not exactly this language)
Author
There was a problem hiding this comment.
Ok, I have to change this, it sound better with the indications. Also, do you feel that using PRINT_BEARS as in the PR is an ok option? I feel that it somehow looks ugly (hacky), but on the other hand I feel that the other options either add duplicate code, or are less flexible?
Contributor
There was a problem hiding this comment.
I don't think it looks ugly or hacky, for the same reason
on the other hand I feel that the other options either add duplicate code
ishanSrt
reviewed
May 26, 2018
|
|
||
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| \nBased on the configuration options the following |
Contributor
There was a problem hiding this comment.
remove the \n in the front of the messages as it is creating additional blank lines
Contributor
|
@tallicamike this is a good patch although can't make out why you need an |
ishanSrt
reviewed
May 26, 2018
| import logging | ||
| import os | ||
| import sys | ||
| from collections import OrderedDict |
Contributor
There was a problem hiding this comment.
I don't see the use of ordered dict over here
Author
There was a problem hiding this comment.
I used a OrderedDict there because I wanted to make sure that each time coala-quickstart is run, fristly the unused bears are printed and then the used bears. (I wanted a deterministic execution, and, for the user, if he doesn't want to check the full log he'll only get a glimpse at the ones that were chosen - I feel this is the most common case). Sounds ok? Or should I change it?
Contributor
|
manual rebase is needed |
gitmate-bot
reviewed
May 31, 2018
| for label_bears in nonempty_label_bears: | ||
| printer.print(PRINT_BEARS[label_bears]['msg']) | ||
| for language in relevant_bears[label_bears]: | ||
| printer.print(" [" + language + "]", |
There was a problem hiding this comment.
You do not use the preferred quotation marks.
Origin: QuotesBear, Section: all.quotes.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
+++ b/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
@@ -236,7 +236,7 @@
for label_bears in nonempty_label_bears:
printer.print(PRINT_BEARS[label_bears]['msg'])
for language in relevant_bears[label_bears]:
- printer.print(" [" + language + "]",
+ printer.print(' [' + language + "]",
color=PRINT_BEARS[label_bears]["colors"][0])
for bear in relevant_bears[label_bears][language]:
printer.print(" " + bear.name,
gitmate-bot
reviewed
May 31, 2018
| for label_bears in nonempty_label_bears: | ||
| printer.print(PRINT_BEARS[label_bears]['msg']) | ||
| for language in relevant_bears[label_bears]: | ||
| printer.print(" [" + language + "]", |
There was a problem hiding this comment.
You do not use the preferred quotation marks.
Origin: QuotesBear, Section: all.quotes.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
+++ b/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
@@ -236,7 +236,7 @@
for label_bears in nonempty_label_bears:
printer.print(PRINT_BEARS[label_bears]['msg'])
for language in relevant_bears[label_bears]:
- printer.print(" [" + language + "]",
+ printer.print(" [" + language + ']',
color=PRINT_BEARS[label_bears]["colors"][0])
for bear in relevant_bears[label_bears][language]:
printer.print(" " + bear.name,
gitmate-bot
reviewed
May 31, 2018
| printer.print(PRINT_BEARS[label_bears]['msg']) | ||
| for language in relevant_bears[label_bears]: | ||
| printer.print(" [" + language + "]", | ||
| color=PRINT_BEARS[label_bears]["colors"][0]) |
There was a problem hiding this comment.
You do not use the preferred quotation marks.
Origin: QuotesBear, Section: all.quotes.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
+++ b/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
@@ -237,7 +237,7 @@
printer.print(PRINT_BEARS[label_bears]['msg'])
for language in relevant_bears[label_bears]:
printer.print(" [" + language + "]",
- color=PRINT_BEARS[label_bears]["colors"][0])
+ color=PRINT_BEARS[label_bears]['colors'][0])
for bear in relevant_bears[label_bears][language]:
printer.print(" " + bear.name,
color=PRINT_BEARS[label_bears]["colors"][1])
gitmate-bot
reviewed
May 31, 2018
| printer.print(" [" + language + "]", | ||
| color=PRINT_BEARS[label_bears]["colors"][0]) | ||
| for bear in relevant_bears[label_bears][language]: | ||
| printer.print(" " + bear.name, |
There was a problem hiding this comment.
You do not use the preferred quotation marks.
Origin: QuotesBear, Section: all.quotes.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
+++ b/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
@@ -239,7 +239,7 @@
printer.print(" [" + language + "]",
color=PRINT_BEARS[label_bears]["colors"][0])
for bear in relevant_bears[label_bears][language]:
- printer.print(" " + bear.name,
+ printer.print(' ' + bear.name,
color=PRINT_BEARS[label_bears]["colors"][1])
printer.print("")
gitmate-bot
reviewed
May 31, 2018
| color=PRINT_BEARS[label_bears]["colors"][0]) | ||
| for bear in relevant_bears[label_bears][language]: | ||
| printer.print(" " + bear.name, | ||
| color=PRINT_BEARS[label_bears]["colors"][1]) |
There was a problem hiding this comment.
You do not use the preferred quotation marks.
Origin: QuotesBear, Section: all.quotes.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
+++ b/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
@@ -240,7 +240,7 @@
color=PRINT_BEARS[label_bears]["colors"][0])
for bear in relevant_bears[label_bears][language]:
printer.print(" " + bear.name,
- color=PRINT_BEARS[label_bears]["colors"][1])
+ color=PRINT_BEARS[label_bears]['colors'][1])
printer.print("")
gitmate-bot
reviewed
May 31, 2018
| for bear in relevant_bears[label_bears][language]: | ||
| printer.print(" " + bear.name, | ||
| color=PRINT_BEARS[label_bears]["colors"][1]) | ||
| printer.print("") |
There was a problem hiding this comment.
You do not use the preferred quotation marks.
Origin: QuotesBear, Section: all.quotes.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
+++ b/tmp/tmpswjp7wec/coala_quickstart/generation/Bears.py
@@ -241,7 +241,7 @@
for bear in relevant_bears[label_bears][language]:
printer.print(" " + bear.name,
color=PRINT_BEARS[label_bears]["colors"][1])
- printer.print("")
+ printer.print('')
def generate_requirements_map(bears):
gitmate-bot
reviewed
May 31, 2018
| [('Python', 70), ('Unknown', 30)], self.printer, | ||
| self.arg_parser, {})) | ||
| print_relevant_bears(self.printer, OrderedDict([('unusable', {}), | ||
| ('usable', filter_relevant_bears([('Python', 70), ('Unknown', 30)], |
There was a problem hiding this comment.
The code does not comply to PEP8.
Origin: PEP8Bear, Section: all.autopep8.
The issue can be fixed by applying the following patch:
--- a/tmp/tmpswjp7wec/tests/generation/Bears.py
+++ b/tmp/tmpswjp7wec/tests/generation/Bears.py
@@ -288,8 +288,8 @@
def test_print_relevant_bears(self):
with retrieve_stdout() as custom_stdout:
print_relevant_bears(self.printer, OrderedDict([('unusable', {}),
- ('usable', filter_relevant_bears([('Python', 70), ('Unknown', 30)],
- self.printer, self.arg_parser, {}))]))
+ ('usable', filter_relevant_bears([('Python', 70), ('Unknown', 30)],
+ self.printer, self.arg_parser, {}))]))
self.assertIn("PycodestyleBear", custom_stdout.getvalue())
# Should print only the usable bears
self.assertNotIn("unusable",
gitmate-bot
reviewed
May 31, 2018
| self.arg_parser, {})) | ||
| print_relevant_bears(self.printer, OrderedDict([('unusable', {}), | ||
| ('usable', filter_relevant_bears([('Python', 70), ('Unknown', 30)], | ||
| self.printer, self.arg_parser, {}))])) |
There was a problem hiding this comment.
E128 continuation line under-indented for visual indent
Origin: PycodestyleBear (E128), Section: all.autopep8.
gitmate-bot
reviewed
May 31, 2018
| def test_print_relevant_bears_no_bears(self): | ||
| with retrieve_stdout() as custom_stdout: | ||
| print_relevant_bears(self.printer, OrderedDict([('unusable', {}), | ||
| ('usable', {})])) |
There was a problem hiding this comment.
E128 continuation line under-indented for visual indent
Origin: PycodestyleBear (E128), Section: all.autopep8.
gitmate-bot
reviewed
May 31, 2018
| self.assertNotIn("usable", | ||
| # Should print only the usable bears | ||
| self.assertIn("usable", | ||
| custom_stdout.getvalue()) |
There was a problem hiding this comment.
E127 continuation line over-indented for visual indent
Origin: PycodestyleBear (E127), Section: all.autopep8.
gitmate-bot
reviewed
May 31, 2018
| [('Python', 70), ('Unknown', 30)], self.printer, | ||
| self.arg_parser, {})) | ||
| print_relevant_bears(self.printer, OrderedDict([('unusable', {}), | ||
| ('usable', filter_relevant_bears([('Python', 70), ('Unknown', 30)], |
There was a problem hiding this comment.
Line is longer than allowed. (83 > 80)
Origin: LineLengthBear, Section: all.linelength.
Travis tests have failedHey @tallicamike, 3rd Buildcoala --non-interactive |
tallicamike
commented
May 31, 2018
| printer.print(BEAR_HELP) | ||
|
|
||
| # Don't print anything for empty bear sets | ||
| nonempty_label_bears = ( |
Author
There was a problem hiding this comment.
Should we print something if there are no relevant bears?
Contributor
There was a problem hiding this comment.
i am sure that would never be the case 😉
Member
There was a problem hiding this comment.
yeah, imo a simple print statement saying no relevant bears found should suffice (just in case there's a change of design in future which actually allows no relevant bears, we don't want user to be confused when he sees an empy screen with no message printed)
ishanSrt
reviewed
May 31, 2018
|
|
||
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| Based on the configuration options the following bears have been identified |
ishanSrt
reviewed
May 31, 2018
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| Based on the configuration options the following bears have been identified | ||
| as unusable ('-C' or '--ci' set non-interactive mode, making the bears that |
ishanSrt
reviewed
May 31, 2018
| Based on the configuration options the following bears have been identified | ||
| as unusable ('-C' or '--ci' set non-interactive mode, making the bears that | ||
| need to be configured by the user unusable: check '--allow-incomplete-sections' | ||
| for more information): |
Contributor
There was a problem hiding this comment.
or don't run in non interactive mode
ishanSrt
reviewed
May 31, 2018
| print_relevant_bears(self.printer, OrderedDict([('unusable', {}), | ||
| ('usable', {})])) | ||
| self.assertNotIn("usable", custom_stdout.getvalue()) | ||
| # Should print only the usable bears |
Contributor
There was a problem hiding this comment.
no output is being printed in this case right?
this comment seems misleading because of that.
ishanSrt
reviewed
Jun 5, 2018
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| Based on the configuration options the following bears have been identified | ||
| as unusable (options '-C' or '--ci' (non interactive mode) make the bears that |
Contributor
There was a problem hiding this comment.
language trouble, parenthesis shouldn't be here starting before the options
Member
There was a problem hiding this comment.
imo, the sentence is too long too, needs to split.
Member
There was a problem hiding this comment.
currently, there's nesting of braces, and the sentence inside the braces is long too.
Author
There was a problem hiding this comment.
How about this: "The following bears have been dropped because '-C'/'--ci' (non interactive mode) option has ben selected. Remove the option or check '--allow-incomplete-sections' option for more information on how to include them:" ?
ishanSrt
approved these changes
Jun 5, 2018
Contributor
ishanSrt
left a comment
ishanSrt
left a comment
There was a problem hiding this comment.
LGTM ping @satwikkansal
satwikkansal
requested changes
Jun 17, 2018
| """.format(BEAR_DOCS_URL) | ||
|
|
||
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ |
Member
There was a problem hiding this comment.
A slight indentation issue, can you move the opening brace to previous line
PRINT_BEARS = {'unusable': {| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| Based on the configuration options the following bears have been identified | ||
| as unusable (options '-C' or '--ci' (non interactive mode) make the bears that |
Member
There was a problem hiding this comment.
imo, the sentence is too long too, needs to split.
| PRINT_BEARS = {'unusable': | ||
| {'msg': """ | ||
| Based on the configuration options the following bears have been identified | ||
| as unusable (options '-C' or '--ci' (non interactive mode) make the bears that |
Member
There was a problem hiding this comment.
currently, there's nesting of braces, and the sentence inside the braces is long too.
| Based on the configuration options the following bears have been identified | ||
| as unusable (options '-C' or '--ci' (non interactive mode) make the bears that | ||
| need to be configured by the user unusable: don't run in non interactive mode | ||
| or check '--allow-incomplete-sections' for more information on how to include |
Member
There was a problem hiding this comment.
"or check '--allow-incomplete-sections' option for more information...
Author
There was a problem hiding this comment.
add "option" to it, that is?
| or check '--allow-incomplete-sections' for more information on how to include | ||
| the unusable bears): | ||
| """, | ||
| 'colors': ('green', 'red')}, |
Member
There was a problem hiding this comment.
There seems like an indentation issue in the entire dict, can you please check that?
Author
There was a problem hiding this comment.
The linter isn't complaining, but indeed it is hard to read. Re-writing it nicely kind of beats me, what I would suggest would be to add variables for each 'msg' value of the dictionary so it should be more "readable". (or another option would be to create variables for the inner dictionaries - 'unusable' and 'usable')
I'm thinking of something like this:
PRINT_BEARS = {
'unusable': {
'msg': PRINT_UNUSABLE,
'colors': ('green', 'red')
},
What are your thoughts on this?
| print_relevant_bears(printer, relevant_bears) | ||
|
|
||
| # Drop unusable bears | ||
| relevant_bears = relevant_bears['usable'] |
Member
There was a problem hiding this comment.
Isn't the remove_unusable_bears method doing this?
Member
There was a problem hiding this comment.
Author
There was a problem hiding this comment.
I changed remove_unusable_bears to also add the unusable bears to the OrderdDict, but I'm using that only for printing purposes. Here the printing has ended, so I drop unusable bears from the dict.
Member
There was a problem hiding this comment.
I changed remove_unusable_bears to also add the unusable bears to the OrderdDict
doesn't that contradict with the function name? remove_unusable_bears is adding unusable bears to OrdereDict
| print_relevant_bears(printer, relevant_bears) | ||
| # OrderedDict used for print_relevant_bears to first print unusable bears | ||
| relevant_bears = OrderedDict( | ||
| [('unusable', {}), |
Member
There was a problem hiding this comment.
Not sure if I full understand this, where are we populating the dictionary with key unusable 😅
Author
There was a problem hiding this comment.
see remove_unusable_bears in this PR
| printer.print(BEAR_HELP) | ||
|
|
||
| # Don't print anything for empty bear sets | ||
| nonempty_label_bears = ( |
Member
There was a problem hiding this comment.
yeah, imo a simple print statement saying no relevant bears found should suffice (just in case there's a change of design in future which actually allows no relevant bears, we don't want user to be confused when he sees an empy screen with no message printed)
| @@ -200,38 +200,48 @@ def get_non_optional_settings_bears(bears): | |||
|
|
|||
| def remove_unusable_bears(bears, unusable_bears): | |||
Member
There was a problem hiding this comment.
I think based on the new changes, a better name could be filter_unsuable_bears
Author
There was a problem hiding this comment.
Oh, you are right! I didn't thought about that! Makes more sense now! Thank you!
Remove duplicate printing of `relevant_bears` and instead change `print_relevant_bears` to inform the user both of the usable and unusable bears. Closes coala#220
jayvdb
requested changes
Aug 1, 2018
| printer.print('') | ||
| printer.print(BEAR_HELP) | ||
|
|
||
| nonempty_label_bears = [ |
|
|
||
| if not nonempty_label_bears: | ||
| printer.print('No relevant bears were found.') | ||
| else: |
Member
There was a problem hiding this comment.
you can use return here if there are no bears to print
then the following can be dedented
| PRINT_BEARS = { | ||
| 'unusable': { | ||
| 'msg': PRINT_UNUSABLE, | ||
| 'colors': ('green', 'red') |
|
|
||
| print_relevant_bears(printer, relevant_bears) | ||
| # OrderedDict used for print_relevant_bears to first print unusable bears | ||
| relevant_bears = OrderedDict( |
Member
There was a problem hiding this comment.
labelled_bears = {'usable': selected_bears}
print_labels = ['usable']| extracted_information = collect_info(project_dir) | ||
|
|
||
| relevant_bears = filter_relevant_bears( | ||
| selected_bears = filter_relevant_bears( |
|
|
||
|
|
||
| def remove_unusable_bears(bears, unusable_bears): | ||
| def filter_unusable_bears(bears, unusable_bears): |
Member
There was a problem hiding this comment.
This can stay as remove_unusable_bears , and return the removed bears grouped by language.
| if bear in unusable_bears: | ||
| bears[language].remove(bear) | ||
| bears['usable'][language].remove(bear) | ||
| bears['unusable'][language] = bears['unusable'].get( |
Member
There was a problem hiding this comment.
return a new dict with the unusable bears grouped by language.
|
|
||
|
|
||
| def print_relevant_bears(printer, relevant_bears, label='relevant'): | ||
| def print_relevant_bears(printer, relevant_bears): |
Member
There was a problem hiding this comment.
change this to:
def print_bears(printer, labelled_bears, labels=['usable']):
The order of the labels is then used to determine which groups to show.
| print_relevant_bears(printer, relevant_bears, 'usable') | ||
| unusable_bears = get_non_optional_settings_bears( | ||
| relevant_bears['usable']) | ||
| filter_unusable_bears(relevant_bears, unusable_bears) |
Member
There was a problem hiding this comment.
labelled_bears['unusable'] = remove_unusable_bears(relevant_bears, unusable_bears)
print_labels = ['unusable', 'usable']| relevant_bears['usable']) | ||
| filter_unusable_bears(relevant_bears, unusable_bears) | ||
|
|
||
| print_relevant_bears(printer, relevant_bears) |
Member
There was a problem hiding this comment.
print_labelled_bears(print, labelled_bears, print_labels)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove duplicate printing of
relevant_bears`` and instead changeprint_relevant_bears` to inform the user both of the usable andunusable bears.
Closes #220