Section Tags Filter #5690

ksdme · 2018-08-05T15:20:05Z

jayvdb · 2018-08-05T15:35:45Z

coalib/parsing/DefaultArgParser.py

@@ -177,6 +177,10 @@ def default_arg_parser(formatter_class=None):
        '-d', '--bear-dirs', type=PathArg, nargs='+', metavar='DIR',
        help='additional directories which may contain bears')

+    inputs_group.add_argument(


This belongs before --bear-dirs , probably after --files .

Lets wait for more feedback on position

Currently,

Inputs: -b, --bears names of bears to use -f, --files files that should be checked -i, --ignore files that should be ignored --limit-files filter the `--files` argument's matches further -d, --bear-dirs additional directories which may contain bears -t, --tags tags of sections to execute

I think tags should definitely be above bear-dirs.

jayvdb · 2018-08-05T15:37:04Z

coalib/parsing/FilterHelper.py

+    :param filter_args:
+        Arguments of the filter to be passed in.
+        For example:
+        ``['tags', ('java',)]``


please use exampes from your cEP, and your own real needs.

this looks like language , not a tag which should be an arbitrary grouping which couldnt be achieved via any other means.

I used them as examples, but I will update it.

jayvdb · 2018-08-05T15:38:06Z

coalib/parsing/FilterHelper.py

+    :param filters:
+        List of args based on ``sections`` has to be filtered.
+        For example:
+        ``[['tags', ('save', 'change')], ['exclude', ('css', '!python')]]``


again, dont use tags that are a language . it is confusing.

jayvdb · 2018-08-05T15:41:19Z

tests/settings/ConfigurationGatheringTest.py

@@ -198,12 +198,16 @@ def test_merge_defaults(self):
                gather_configuration(lambda *args: True,
                                     self.log_printer,
                                     arg_list=['-S', 'value=1', 'test.value=2',
-                                               '-c', escape(temporary, '\\')] +
+                                               '--tags', 'save', '-c',


move the '-c', back to the next line, so you dont modify that line.

it is also confusing to have the '-c', on this line when its arg is on the next line.

gitmate-bot · 2018-08-05T16:00:43Z

tests/settings/ConfigurationGatheringTest.py

@@ -198,12 +198,16 @@ def test_merge_defaults(self):
                gather_configuration(lambda *args: True,
                                     self.log_printer,
                                     arg_list=['-S', 'value=1', 'test.value=2',
-                                               '-c', escape(temporary, '\\')] +
+                                               '-c', escape(temporary, '\\'),
+                                               '--tags', 'save',] +


The code does not comply to PEP8.

Origin: PEP8Bear, Section: autopep8.

The issue can be fixed by applying the following patch:

--- a/tmp/tmpbnwihg5y/tests/settings/ConfigurationGatheringTest.py +++ b/tmp/tmpbnwihg5y/tests/settings/ConfigurationGatheringTest.py @@ -199,7 +199,7 @@ self.log_printer, arg_list=['-S', 'value=1', 'test.value=2', '-c', escape(temporary, '\\'), - '--tags', 'save',] + + '--tags', 'save', ] + self.min_args)) self.assertEqual(sections['cli'],

gitmate-bot · 2018-08-05T16:00:44Z

tests/settings/ConfigurationGatheringTest.py

@@ -198,12 +198,16 @@ def test_merge_defaults(self):
                gather_configuration(lambda *args: True,
                                     self.log_printer,
                                     arg_list=['-S', 'value=1', 'test.value=2',
-                                               '-c', escape(temporary, '\\')] +
+                                               '-c', escape(temporary, '\\'),
+                                               '--tags', 'save',] +


E231 missing whitespace after ','

Origin: PycodestyleBear (E231), Section: autopep8.

jayvdb · 2018-08-06T02:54:27Z

coalib/parsing/FilterHelper.py



 def is_valid_filter(filter):
    return filter in available_filters


+def is_valid_section_filter(filter):


nope.

lets improve the filters system to support sections.

def foo_filter(thing, args):

every filter than checks whether is knows what is in thing, e.g. using isinstance , and raises NotImplementedError if it doesnt get it. Existing filters only know about Bear.

You make a small chance to the existing filters to make them compliant with the new API by adding a raise NotImplementedError to them.

(Actually most of them understand could easily be improved to understand Section, but that is beyond the scope of your change, and each will need to be carefully done wrt the CLI args to ensure we build a good user experience)

jayvdb · 2018-08-06T02:57:03Z

coalib/parsing/FilterHelper.py

+    if not filter_args or len(filter_args) == 0:
+        return all_sections
+
+    filter_function = available_section_filters[filter_name]


not needed. below you would catch NotImplementedError and ignore it.

jayvdb · 2018-08-06T03:00:04Z

coalib/parsing/filters/TagSectionFilter.py

+    :return:        ``True`` if this section matches the criteria inside args,
+                    ``False`` otherwise.
+    """
+    if section.name == 'cli':


I agree with @shreyans800755 that this doesnt belong in the filter.
ksdme@f8fc69c#r29964403

The cli section should be kept hidden inside coalib/settings, as a special case implementation detail. It annoys me that we have any UI messages that mention this section -- the user cant see it, so they shouldnt hear about it.

And likewise developers in other parts of coalib shouldnt need to deal with that section.

gaocegege · 2018-08-06T03:54:24Z

coalib/coala_main.py

@@ -159,6 +161,15 @@ def run_coala(console_printer=None,
                (section_name, sections[section_name])
                for section_name in targets)

+        # Collect all the section filters which are based on args,


I think we could remove the last ,

ksdme · 2018-08-06T20:15:22Z

Updated to use --filter-by (can be used by doing coala --filter-by section_tags save) for both section filters and bear filters. Known issues:

Requires update to --filter-by documentation.

shreyans800755 · 2018-08-06T20:50:01Z

coalib/parsing/FilterHelper.py

    """
+    things = bears


Let's call it items rather than things

shreyans800755 · 2018-08-06T20:51:36Z

coalib/parsing/FilterHelper.py

+        List of filters in standard filter format, i.e
+        ``[['filter_name', 'arg1', 'arg2']]``.
+    """
+    if args is None:


Do we have any practical case for this condition ?
If so, is there way we can check this before calling this function ?

run_coala :param arg_parser: Instance of ArgParser that is used to non-setting arguments. :param arg_list: The CLI argument list. :param args: Alternative pre-parsed CLI arguments.

This is not just about the tests, since args to run_coala() can be optionally passed instead of arg_list, I think we need to merge opts from both, so as to enable the internal usage of this filter. Tests also use can use this by directly calling run_coala() with an arg list.

shreyans800755 · 2018-08-06T20:52:04Z

coalib/parsing/FilterHelper.py

+        List of filters in standard filter format, i.e
+        ``[['filter_name', 'arg1', 'arg2']]``.
+    """
+    if args is None:


Because this is not the job of this function. This function is just a getter

It looks like a helper for the tests , so should be moved into the tests.

run_coala :param arg_parser: Instance of ArgParser that is used to non-setting arguments. :param arg_list: The CLI argument list. :param args: Alternative pre-parsed CLI arguments.

This is not just about the tests, since args to run_coala() can be optionally passed instead of arg_list, I think we need to merge opts from both, so as to enable the internal usage of this filter. Tests also use can use this by directly calling run_coala() with an arg list.

jayvdb · 2018-08-06T21:49:53Z

coalib/parsing/FilterHelper.py

@@ -49,22 +50,83 @@ def apply_filter(filter_name, filter_args, all_bears=None):
    return local_bears, global_bears


-def apply_filters(filters, bears=None):
+def _apply_section_filter(filter_name, filter_args, all_sections):


move this after the apply_filters function so that the diff better aligns, and probably at the end as it is a private function

jayvdb · 2018-08-06T21:54:38Z

coalib/parsing/FilterHelper.py

+        List of filters in standard filter format, i.e
+        ``[['filter_name', 'arg1', 'arg2']]``.
+    """
+    if args is None:


It looks like a helper for the tests , so should be moved into the tests.

jayvdb · 2018-08-06T21:58:04Z

coalib/parsing/filters/decorators.py

+from functools import wraps
+
+
+def typed_filter(type_classes, msg=None):


(almost) very generic ... belongs in coala_utils::decorators.py

and this is enforce_duck_type or enforce_duct_tape ? ;-)

and to make calling generic, use

@typed_filter(bear='bearclass') def language_filter(bear, args): ```

belongs in coala_utils::decorators.py

Unfortunately not ...

Because we are not allowed to do imports here! argh.

It was built: https://gitlab.com/coala/coala-utils/merge_requests/82

jayvdb · 2018-08-06T21:58:48Z

coalib/parsing/filters/decorators.py

+    def decorator(filter):
+
+        @wraps(filter)
+        def decorated_filter(thing, *args, **kargs):


thing -> obj or object_

jayvdb · 2018-08-06T22:03:35Z

coalib/parsing/filters/decorators.py

+                    break
+            else:
+                raise NotImplementedError(
+                    msg or '\'{filter}\' filter can only handle {type_name}. '


it was going so well until you mentioned filter here.

the thing you are wrapping has a name, which you are using here.

This decorator is a developer helper ... to catch when they have done the wrong thing. it shouldnt give pretty names ... the caller needs to detect this and make it user-friendly.

Why should a NotImplementedError say "language_filter filter" .. ? language_filter is better, and the backtrace is even better, so no need to tidy it up.

To help, create a subclass of NotImplementedError and stuff into it extra attributes to record what went wrong, such as the callee, expected classes, and the incorrect object which was encountered. Then whoever is catching it can understand what went wrong and create a user-friendly message.

jayvdb · 2018-08-06T22:09:14Z

coalib/parsing/filters/TagSectionFilter.py

+
+
+@typed_filter(('bearclass', 'Bear', 'Section'))
+def tag_section_filter(section_or_bear, args):


section_tag_filter

gitmate-bot · 2018-08-07T11:33:12Z

coalib/parsing/filters/__init__.py

@@ -5,9 +5,11 @@
 from .LanguageFilter import language_filter
 from .CanDetectFilter import can_detect_filter
 from .CanFixFilter import can_fix_filter
+from .SectionTagFilter import section_tag_filter


This file contains unused source code.

Origin: PyUnusedCodeBear, Section: flakes.

The issue can be fixed by applying the following patch:

--- a/tmp/tmp9_9zjivn/coalib/parsing/filters/__init__.py +++ b/tmp/tmp9_9zjivn/coalib/parsing/filters/__init__.py @@ -5,7 +5,6 @@ from .LanguageFilter import language_filter from .CanDetectFilter import can_detect_filter from .CanFixFilter import can_fix_filter -from .SectionTagFilter import section_tag_filter available_filters = {'language': language_filter,

mostly done; no objections

Section filtering based on configurations is now possible. This commit also adds - Typed filters, filters can be decorated to specifically support certain types of things. - Supports section filtering.

The new API requires filters to be type annotated to support for proper usage resolution, this commit updates the filters with that requirement.

SectionTagsFilter can be used with --filter-by to filter sections that are to be executed based on their tags property.

jayvdb · 2018-08-10T10:19:16Z

ack 3818164 ce6513e ba2c2f7

jayvdb · 2018-08-10T10:19:28Z

@gitmate-bot ff

gitmate-bot · 2018-08-10T10:19:30Z

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

gitmate-bot · 2018-08-10T10:20:44Z

Automated fastforward with GitMate.io was successful! 🎉

gitmate-bot added process/pending review size/M labels Aug 5, 2018

jayvdb requested review from Makman2 and shreyans800755 August 5, 2018 15:28

ksdme force-pushed the tag-filters-gsoc branch from 4747fe0 to f6f250b Compare August 5, 2018 15:31

jayvdb requested review from jayvdb, palash25, sangamcse and aashutoshrathi August 5, 2018 15:31

jayvdb requested changes Aug 5, 2018

View reviewed changes

jayvdb requested review from AbdealiLoKo and Udayan12167 August 5, 2018 15:42

jayvdb added process/wip and removed process/pending review labels Aug 5, 2018

ksdme force-pushed the tag-filters-gsoc branch from f6f250b to dc4c0b7 Compare August 5, 2018 15:56

gitmate-bot added process/pending review and removed process/wip labels Aug 5, 2018

gitmate-bot reviewed Aug 5, 2018

View reviewed changes

ksdme force-pushed the tag-filters-gsoc branch from dc4c0b7 to a35236d Compare August 5, 2018 16:06

jayvdb requested changes Aug 6, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Aug 6, 2018

gaocegege reviewed Aug 6, 2018

View reviewed changes

ksdme force-pushed the tag-filters-gsoc branch from a35236d to 7ef41e7 Compare August 6, 2018 20:11

gitmate-bot added process/pending review and removed process/wip labels Aug 6, 2018

ksdme force-pushed the tag-filters-gsoc branch from 7ef41e7 to f765cd7 Compare August 6, 2018 20:24

shreyans800755 suggested changes Aug 6, 2018

View reviewed changes

jayvdb previously requested changes Aug 6, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Aug 6, 2018

ksdme force-pushed the tag-filters-gsoc branch from f765cd7 to da6ca4a Compare August 7, 2018 11:30

gitmate-bot added process/pending review and removed process/wip labels Aug 7, 2018

ksdme changed the title ~~Tag Section Filters~~ Section Tags Filter Aug 7, 2018

gitmate-bot reviewed Aug 7, 2018

View reviewed changes

ksdme force-pushed the tag-filters-gsoc branch 2 times, most recently from 726414c to 93efd22 Compare August 7, 2018 12:06

gaocegege approved these changes Aug 7, 2018

View reviewed changes

jayvdb approved these changes Aug 10, 2018

View reviewed changes

ksdme added 3 commits August 10, 2018 15:28

filters: Add support for Section Filtering

3818164

Section filtering based on configurations is now possible. This commit also adds - Typed filters, filters can be decorated to specifically support certain types of things. - Supports section filtering.

filters: Update filters to new API

ce6513e

The new API requires filters to be type annotated to support for proper usage resolution, this commit updates the filters with that requirement.

SectionTagsFilter: Add SectionTagsFilter

ba2c2f7

SectionTagsFilter can be used with --filter-by to filter sections that are to be executed based on their tags property.

ksdme force-pushed the tag-filters-gsoc branch from 93efd22 to ba2c2f7 Compare August 10, 2018 09:59

gitmate-bot added process/approved The PR is approved and will be merged soon and removed process/pending review labels Aug 10, 2018

gitmate-bot removed the process/approved The PR is approved and will be merged soon label Aug 10, 2018

gitmate-bot merged commit ba2c2f7 into coala:master Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Section Tags Filter #5690

Section Tags Filter #5690

ksdme commented Aug 5, 2018 •

edited

jayvdb Aug 5, 2018

ksdme Aug 5, 2018

jayvdb Aug 5, 2018

ksdme Aug 5, 2018

jayvdb Aug 5, 2018

jayvdb Aug 5, 2018

gitmate-bot Aug 5, 2018

gitmate-bot Aug 5, 2018

jayvdb Aug 6, 2018

jayvdb Aug 6, 2018

jayvdb Aug 6, 2018

gaocegege Aug 6, 2018

ksdme commented Aug 6, 2018 •

edited

shreyans800755 Aug 6, 2018

shreyans800755 Aug 6, 2018

ksdme Aug 7, 2018

shreyans800755 Aug 6, 2018

jayvdb Aug 6, 2018

ksdme Aug 7, 2018

jayvdb Aug 6, 2018

jayvdb Aug 6, 2018

jayvdb Aug 6, 2018

jayvdb Aug 10, 2018

jayvdb Aug 6, 2018

jayvdb Aug 6, 2018

jayvdb Aug 6, 2018

gitmate-bot Aug 7, 2018

jayvdb commented Aug 10, 2018

jayvdb commented Aug 10, 2018

gitmate-bot commented Aug 10, 2018

gitmate-bot commented Aug 10, 2018

		from functools import wraps


		def typed_filter(type_classes, msg=None):



		@typed_filter(('bearclass', 'Bear', 'Section'))
		def tag_section_filter(section_or_bear, args):

Section Tags Filter #5690

Section Tags Filter #5690

Conversation

ksdme commented Aug 5, 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

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

ksdme commented Aug 6, 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

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 Aug 10, 2018

jayvdb commented Aug 10, 2018

gitmate-bot commented Aug 10, 2018

gitmate-bot commented Aug 10, 2018

ksdme commented Aug 5, 2018 •

edited

ksdme commented Aug 6, 2018 •

edited