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 whitelist feature to zero plugin #1641

Merged
merged 5 commits into from Oct 17, 2015
Merged

Conversation

adkow
Copy link
Contributor

@adkow adkow commented Oct 9, 2015

Hi, I added new behavior to zero plugin as requested in #1621.
It works as expected and with no errors (for me 😄).
Now, zero has new option keep_fields which contains fields that user want to preserve. Everything else is cleared.

@sampsyo
Copy link
Member

sampsyo commented Oct 9, 2015

Awesome! This looks like it's on the right track.

Would you mind adding some documentation on how the option works to the documentation for the zero plugin?

It would also be very satisfying to have a few tests in the test_zero.py file that show that this works how you want it to.

Finally, it looks like flake8 has a few style suggestions.

# These fields should be preserved
if 'id' in self.patterns: del self.patterns['id']
if 'path' in self.patterns: del self.patterns['path']
if 'album_id' in self.patterns: del self.patterns['album_id']
Copy link
Member

Choose a reason for hiding this comment

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

May I recommend a little loop?

for key in ('id', 'path', 'album_id'):
    if key in self.patterns:
        del self.patterns[key]

@sampsyo
Copy link
Member

sampsyo commented Oct 9, 2015

Also: It would be awesome to avoid duplicating code between the two modes. Can you please factor out the shared logic to a helper function?

@adkow
Copy link
Contributor Author

adkow commented Oct 9, 2015

Thanks for advices, I'm working on it 👍.

@adkow
Copy link
Contributor Author

adkow commented Oct 9, 2015

I edited code as you suggested and added some lines to documentation. I hope it'll be fine.
However, I'm a beginner in Python and not very familiar with its tests concept, so I wasn't able to write test for new zero behavior. Sorry about that, I'm sure someone will do that with ease.

@@ -41,26 +41,50 @@ def __init__(self):

self.config.add({
'fields': [],
'update_database': False,
'keep_fields': [],
'update_database': False
Copy link
Member

Choose a reason for hiding this comment

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

Tiny style issue: please add a trailing comma, which helps with maintainability.

@sampsyo
Copy link
Member

sampsyo commented Oct 12, 2015

Looking great! I have a few minor suggestions, but afterward, this should be ready to merge. ✨ Thank you for your effort on this!

@adkow
Copy link
Contributor Author

adkow commented Oct 12, 2015

Done 👍.

@ghost
Copy link

ghost commented Oct 16, 2015

When will this be merged?

@sampsyo
Copy link
Member

sampsyo commented Oct 16, 2015

As soon as I have a moment. Thanks for your patience, @flok3r.

@sampsyo sampsyo merged commit d180d42 into beetbox:master Oct 17, 2015
sampsyo added a commit that referenced this pull request Oct 17, 2015
Add whitelist feature to zero plugin
sampsyo added a commit that referenced this pull request Oct 17, 2015
The old version would trigger the warning twice.
sampsyo added a commit that referenced this pull request Oct 17, 2015
@sampsyo
Copy link
Member

sampsyo commented Oct 17, 2015

Thank you again, @adkow! This is all merged up. ✨

@adkow adkow deleted the zero-whitelist branch October 17, 2015 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants