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

Add --tags and --skip-tags options #81

Merged
merged 1 commit into from
Feb 11, 2019
Merged

Conversation

seleznev
Copy link
Collaborator

@seleznev seleznev commented Feb 5, 2019

Иллюстрация к #80.

@seleznev seleznev added the enhancement New feature or request label Feb 5, 2019
@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #81 into master will increase coverage by 0.11%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   84.85%   84.97%   +0.11%     
==========================================
  Files          12       12              
  Lines        1182     1211      +29     
==========================================
+ Hits         1003     1029      +26     
- Misses        179      182       +3
Impacted Files Coverage Δ
k8s_handle/settings.py 100% <100%> (ø) ⬆️
k8s_handle/__init__.py 37.38% <50%> (+0.48%) ⬆️
k8s_handle/templating.py 93.65% <95.65%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2d78ce...363bf0c. Read the comment docs.

Copy link
Collaborator

@rvadim rvadim left a comment

Choose a reason for hiding this comment

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

Good, but some comments. Changes up to you.

k8s_handle/__init__.py Outdated Show resolved Hide resolved
k8s_handle/templating.py Outdated Show resolved Hide resolved
@@ -113,6 +116,40 @@ def generate_by_context(self, context):
raise TemplateRenderingError('Unable to render {}, due to: {}'.format(template, e))
return output

def _filter_tagged_templates(self, templates, only_tags, skip_tags):
return [i for i in templates if self._evaluate_tags(i, only_tags, skip_tags)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

More idiomatic way:

return templates.filter(lambda i: self._evaluate_tags(i, only_tags, skip_tags)) 

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, maybe def _filter_tagged_templates(self, templates, only_tags, skip_tags), not required at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I replaced _filter_tagged_templates() to filter() + lambda:

templates = filter(
    lambda i: self._evaluate_tags(self._get_template_tags(i),
                                  settings.ONLY_TAGS,
                                  settings.SKIP_TAGS),
    templates
)

def _filter_tagged_templates(self, templates, only_tags, skip_tags):
return [i for i in templates if self._evaluate_tags(i, only_tags, skip_tags)]

def _evaluate_tags(self, template, only_tags, skip_tags):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The method return true or false, maybe _is_template_tagged(...) more accurate name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like both names actually (especially after moving "template" part into separate function). 🙁

Please let me know if _is_template_tagged(self, tags, only_tags, skip_tags) still looks better for you after last changes.

return [i for i in templates if self._evaluate_tags(i, only_tags, skip_tags)]

def _evaluate_tags(self, template, only_tags, skip_tags):
if 'tags' in template:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to separate template tags analysis and skipping logic to 2 different functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved first part into _get_template_tags().

@rvadim
Copy link
Collaborator

rvadim commented Feb 6, 2019

Also, don't forget to add description about this feature in README.md

Copy link
Collaborator

@rvadim rvadim left a comment

Choose a reason for hiding this comment

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

LGTM

@dekhtyarev dekhtyarev merged commit 240eb7b into 2gis:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants