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.

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

No need of --files flag with only global bears #4997

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ishanSrt
Copy link
Contributor

@ishanSrt ishanSrt commented Dec 21, 2017

Fixes #4928

Explanation:
Lines 506-515: Check if only global bears are present and pass around variable only_global_bears
Lines 191-193: Check if only global bears are present and and only the files param is missing. Don't System Exit in that case
Lines 215-219: In the above mentioned case don't print out warning message

So if --files is not provided and only global bears are used, coala operates on nothing

Copy link
Contributor

@pradeepgangwar pradeepgangwar left a comment

Choose a reason for hiding this comment

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

You commit message needs modification:
Please read this guide and try to modify commit message. :)

@ishanSrt
Copy link
Contributor Author

@pradeepgangwar be more precise.

@pradeepgangwar
Copy link
Contributor

quoting it from the link i mentioned in previous comment.

A commit message consists of 3 parts:

  • shortlog
  • commit body
  • issue reference

your commit message should follow this style.

@ishanSrt
Copy link
Contributor Author

@pradeepgangwar commit body is optional only to describe the things that are not self explanatory. Read that entire doc, carefully.

save_sections(sections)
warn_nonexistent_targets(targets, sections)
warn_nonexistent_targets(targets, sections, only_global_bears)
Copy link
Contributor

@pradeepgangwar pradeepgangwar Dec 22, 2017

Choose a reason for hiding this comment

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

I think you should add global_bears parameter to gather_configuration function too, where it calls warn_nonexistent_targets and then see build log, they fail because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is meant by global bears option?

Copy link
Contributor Author

@ishanSrt ishanSrt Dec 22, 2017

Choose a reason for hiding this comment

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

the checks are failing because of the test cases. all bears till now have to have a --files tag. now I am changing that so the tests yell at me why didn't coala quit when the --files was not specified

@gitmate-bot
Copy link
Collaborator

Comment on e080761, file coalib/settings/ConfigurationGathering.py, line 173.

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfrcy4o9m/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmpfrcy4o9m/coalib/settings/ConfigurationGathering.py
@@ -170,7 +170,7 @@
     :param targets:           The targets to check.
     :param sections:          The sections to search. (Dict.)
     :param log_printer:       The log printer to warn to.
-    :param only_global_bears: True if only global bears are 
+    :param only_global_bears: True if only global bears are
                               are being run
     """
     for target in targets:

@gitmate-bot
Copy link
Collaborator

Comment on e080761, file coalib/settings/ConfigurationGathering.py, line 190.

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpfrcy4o9m/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmpfrcy4o9m/coalib/settings/ConfigurationGathering.py
@@ -187,7 +187,7 @@
     bears_config_absent, arg_bears = warn_config_absent(sections,
                                                         ['bears', 'aspects'],
                                                         only_global_bears)
-    if (files_config_absent or bears_config_absent) and (not (only_global_bears and 
+    if (files_config_absent or bears_config_absent) and (not (only_global_bears and
         files_config_absent and arg_files==['files'])) :
         raise SystemExit(2)  # Invalid CLI options provided
 

@gitmate-bot
Copy link
Collaborator

Comment on e080761, file coalib/settings/ConfigurationGathering.py, line 165.

Line is longer than allowed. (85 > 80)

Origin: LineLengthBear, Section: linelength.

@gitmate-bot
Copy link
Collaborator

Comment on e080761, file coalib/settings/ConfigurationGathering.py, line 190.

Line is longer than allowed. (84 > 80)

Origin: LineLengthBear, Section: linelength.

@gitmate-bot
Copy link
Collaborator

Comment on e080761.

Shortlog of the HEAD commit contains 66 character(s). This is 16 character(s) longer than the limit (66 > 50).

Origin: GitCommitBear, Section: commit.

@@ -162,14 +162,16 @@ def save_sections(sections):
conf_writer.close()


def warn_nonexistent_targets(targets, sections, log_printer=None):
def warn_nonexistent_targets(targets, sections, only_global_bears, log_printer=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is longer than allowed. (85 > 80)

Origin: LineLengthBear, Section: linelength.

bears_config_absent, arg_bears = warn_config_absent(sections,
['bears', 'aspects'],
only_global_bears)
if (files_config_absent or bears_config_absent) and (not (only_global_bears and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is longer than allowed. (83 > 80)

Origin: LineLengthBear, Section: linelength.

:param targets: The targets to check.
:param sections: The sections to search. (Dict.)
:param log_printer: The log printer to warn to.
:param only_global_bears: True if only global bears are
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
@@ -170,7 +170,7 @@
     :param targets:           The targets to check.
     :param sections:          The sections to search. (Dict.)
     :param log_printer:       The log printer to warn to.
-    :param only_global_bears: True if only global bears are 
+    :param only_global_bears: True if only global bears are
                               are being run
     """
     for target in targets:

bears_config_absent, arg_bears = warn_config_absent(sections,
['bears', 'aspects'],
only_global_bears)
if (files_config_absent or bears_config_absent) and (not (only_global_bears and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
@@ -187,7 +187,7 @@
     bears_config_absent, arg_bears = warn_config_absent(sections,
                                                         ['bears', 'aspects'],
                                                         only_global_bears)
-    if (files_config_absent or bears_config_absent) and (not (only_global_bears and 
+    if (files_config_absent or bears_config_absent) and (not (only_global_bears and
         files_config_absent and arg_files==['files'])) :
         raise SystemExit(2)  # Invalid CLI options provided
 

@@ -162,14 +162,16 @@ def save_sections(sections):
conf_writer.close()


def warn_nonexistent_targets(targets, sections, log_printer=None):
def warn_nonexistent_targets(targets, sections, only_global_bears, log_printer=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is longer than allowed. (85 > 80)

Origin: LineLengthBear, Section: linelength.

bears_config_absent, arg_bears = warn_config_absent(sections,
['bears', 'aspects'],
only_global_bears)
if (files_config_absent or bears_config_absent) and (not (only_global_bears and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line is longer than allowed. (84 > 80)

Origin: LineLengthBear, Section: linelength.

@@ -162,14 +162,17 @@ def save_sections(sections):
conf_writer.close()


def warn_nonexistent_targets(targets, sections, log_printer=None):
def warn_nonexistent_targets(targets, sections,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5wvtkcjo/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmp5wvtkcjo/coalib/settings/ConfigurationGathering.py
@@ -162,7 +162,7 @@
     conf_writer.close()
 
 
-def warn_nonexistent_targets(targets, sections, 
+def warn_nonexistent_targets(targets, sections,
                              only_global_bears, log_printer=None):
     """
     Prints out a warning on the given log printer for all targets that are

@@ -162,14 +162,17 @@ def save_sections(sections):
conf_writer.close()


def warn_nonexistent_targets(targets, sections, log_printer=None):
def warn_nonexistent_targets(targets, sections,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp5wvtkcjo/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmp5wvtkcjo/coalib/settings/ConfigurationGathering.py
@@ -162,7 +162,7 @@
     conf_writer.close()
 
 
-def warn_nonexistent_targets(targets, sections, 
+def warn_nonexistent_targets(targets, sections,
                              only_global_bears, log_printer=None):
     """
     Prints out a warning on the given log printer for all targets that are

@@ -162,14 +162,17 @@ def save_sections(sections):
conf_writer.close()


def warn_nonexistent_targets(targets, sections, log_printer=None):
def warn_nonexistent_targets(targets, sections,
Copy link
Collaborator

Choose a reason for hiding this comment

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

W291 trailing whitespace

Origin: PycodestyleBear (W291), Section: autopep8.

:param targets: The targets to check.
:param sections: The sections to search. (Dict.)
:param log_printer: The log printer to warn to.
:param only_global_bears: True if only global bears are
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
@@ -170,7 +170,7 @@
     :param targets:           The targets to check.
     :param sections:          The sections to search. (Dict.)
     :param log_printer:       The log printer to warn to.
-    :param only_global_bears: True if only global bears are 
+    :param only_global_bears: True if only global bears are
                               are being run
     """
     for target in targets:

bears_config_absent, arg_bears = warn_config_absent(sections,
['bears', 'aspects'],
only_global_bears)
if (files_config_absent or bears_config_absent) and (not (only_global_bears and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • Trailing whitespaces.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
+++ b/tmp/tmpft6pan2z/coalib/settings/ConfigurationGathering.py
@@ -187,7 +187,7 @@
     bears_config_absent, arg_bears = warn_config_absent(sections,
                                                         ['bears', 'aspects'],
                                                         only_global_bears)
-    if (files_config_absent or bears_config_absent) and (not (only_global_bears and 
+    if (files_config_absent or bears_config_absent) and (not (only_global_bears and
         files_config_absent and arg_files==['files'])) :
         raise SystemExit(2)  # Invalid CLI options provided
 

@@ -162,14 +162,17 @@ def save_sections(sections):
conf_writer.close()


def warn_nonexistent_targets(targets, sections, log_printer=None):
def warn_nonexistent_targets(targets, sections,
only_global_bears, log_printer=None):
Copy link
Member

Choose a reason for hiding this comment

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

never add parameters in the middle of the list; that breaks the API. Add them to the end.

Copy link
Contributor Author

@ishanSrt ishanSrt Mar 17, 2018

Choose a reason for hiding this comment

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

@jayvdb but positional parameters should be before named parameters. Am I missing something over here?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe make it a keyword parameter and shift it to the end?
However, I think we have done deprecation efforts way too long. We are still having 0.x versions, and as a maintainer I would claim our right to break API (as long as we properly mention it in the release notes) especially because we are officially still "alpha" ^^ Those internal things are likely to be not used outside, and people can still use the older version if they are too lazy to update. If we can pin package versions, others can do the same ;)
But @jayvdb that's a different discussion I'm kicking off here, your decision :)

Copy link
Member

Choose a reason for hiding this comment

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

@ishanSrt , it has to be a keyword parameter, or you need to choose a new function name, ideally with a wrapper with the old function name to maintain compatibility.

@Makman2 , One minor release between breakages , otherwise it creates dependency hell which we cant solve without using pre-releases. The real problem is we havent done a minor release in ages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do it but can you explain me in what context are you all talking about? why does this cause API breakage?

@@ -25,6 +25,7 @@
gather_configuration,
get_filtered_bears,
load_configuration,
warn_nonexistent_targets
Copy link
Member

Choose a reason for hiding this comment

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

add trailing comma, so the next person doesnt need to modify your line.

:param log_printer: The log printer to warn to.
:param targets: The targets to check.
:param sections: The sections to search. (Dict.)
:param only_global_bears: True if only global bears are
Copy link
Member

Choose a reason for hiding this comment

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

wrong position in params order

@@ -179,7 +182,8 @@ def warn_nonexistent_targets(targets, sections, log_printer=None):

# Can't be summarized as python will evaluate conditions lazily, those
# functions have intended side effects though.
files_config_absent = warn_config_absent(sections, 'files')
files_config_absent = warn_config_absent(
Copy link
Member

Choose a reason for hiding this comment

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

all of the rest of this function is useless if only_global_bears, because that implicitly says there is at least one bear, so bears_config_absent can never be True.

Bail out early if bears_config_absent is True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see, all of this function is useless, cause we dont require files and we already know that there is atleast one bear provided by the user if only_global_bears is true

log_printer=self.log_printer)
except SystemExit:
raised = True
self.assertFalse(raised)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line contains following spacing inconsistencies:

  • No newline at EOF.

Origin: SpaceConsistencyBear, Section: python.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpdwb2__cj/tests/settings/ConfigurationGatheringTest.py
+++ b/tmp/tmpdwb2__cj/tests/settings/ConfigurationGatheringTest.py
@@ -515,4 +515,4 @@
                 log_printer=self.log_printer)
         except SystemExit:
             raised = True
-        self.assertFalse(raised)+        self.assertFalse(raised)

log_printer=self.log_printer)
except SystemExit:
raised = True
self.assertFalse(raised)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpdwb2__cj/tests/settings/ConfigurationGatheringTest.py
+++ b/tmp/tmpdwb2__cj/tests/settings/ConfigurationGatheringTest.py
@@ -515,4 +515,4 @@
                 log_printer=self.log_printer)
         except SystemExit:
             raised = True
-        self.assertFalse(raised)+        self.assertFalse(raised)

log_printer=self.log_printer)
except SystemExit:
raised = True
self.assertFalse(raised)
Copy link
Collaborator

Choose a reason for hiding this comment

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

W292 no newline at end of file

Origin: PycodestyleBear (W292), Section: autopep8.

@ishanSrt ishanSrt force-pushed the issue4928final branch 3 times, most recently from 7da74fc to 3226108 Compare April 7, 2018 19:48
@@ -10,6 +10,7 @@

from coalib.bearlib.aspects.Metadata import CommitMessage
from coalib.bearlib.languages import Language
from coalib.bears.GlobalBear import GlobalBear
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpqy4n7gm2/tests/settings/ConfigurationGatheringTest.py
+++ b/tmp/tmpqy4n7gm2/tests/settings/ConfigurationGatheringTest.py
@@ -10,7 +10,6 @@
 
 from coalib.bearlib.aspects.Metadata import CommitMessage
 from coalib.bearlib.languages import Language
-from coalib.bears.GlobalBear import GlobalBear
 from coalib.misc import Constants
 from coalib.parsing.DefaultArgParser import PathArg
 from coalib.settings.Section import Section

@ishanSrt ishanSrt force-pushed the issue4928final branch 4 times, most recently from 1281fe6 to 7d3ae9a Compare April 7, 2018 20:24
sections, local_bears, global_bears, targets = (
gather_configuration(
*args,
arg_list=['-b PyromaBear']))
Copy link
Member

Choose a reason for hiding this comment

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

use a dummy bear, not a real one

gather_configuration(
*args,
arg_list=['-b PyromaBear']))
print(global_bears)
Copy link
Member

Choose a reason for hiding this comment

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

test locally, not as a pull request :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants