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

Added fix_permissions plugin #1098

Merged
merged 7 commits into from
Nov 20, 2014
Merged

Added fix_permissions plugin #1098

merged 7 commits into from
Nov 20, 2014

Conversation

xsteadfastx
Copy link
Contributor

i needed a plugin to fix some file permissions while importing files. so i created it :)


# check permissions
if not check_permissions(path, file_perm):
message = 'There was a problem fixing permission on {}'.format(path)
Copy link
Member

Choose a reason for hiding this comment

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

Please use util.bytestring_path(path) to avoid Unicode errors when logging.

@sampsyo
Copy link
Member

sampsyo commented Nov 17, 2014

Awesome! Thanks for contributing. This is a great little addition, and I'm sure others will be interested too.

If you don't mind my bikeshedding a couple of small details:

  • Our plugin names typically don't have underscores. What do you think about just calling this permissions?
  • Would you mind bringing the comments and docstrings in line with beets' standard style? Namely, use triple-double quotes """, full sentences with a capital letter and a period, no space after the opening """, and the closing """ either right after the period (single line) or on its own line (multi-line docstrings).

@sampsyo
Copy link
Member

sampsyo commented Nov 17, 2014

Oh, one last detail I forgot to mention: we will need to add a documentation page for the plugin under docs/plugins/. It could be quite simple for this one.

@xsteadfastx
Copy link
Contributor Author

im so glad for your feedback. i will work on everything you mentioned :)

@xsteadfastx
Copy link
Contributor Author

so... i think i put all your suggestions into this plugin. im not sure about the documentary page. what do you think?

@xsteadfastx
Copy link
Contributor Author

im trying to get my head around writing tests. for my own tests... like changing permissions and importing them... and see what permissions are on the importet files everything works fine. now i try to write a test and it doesnt work at all. maybe anyone with an idea? i read through some beets plugin testing code and i thought this is how it works. but the files just dont get chmod'ed.


from _common import unittest                                                       
from helper import TestHelper                                                      
from beetsplug.permissions import check_permissions, convert_perm                  


class PermissionsPluginTest(unittest.TestCase, TestHelper):                        
    def setUp(self):                                                               
        self.setup_beets()                                                         
        self.load_plugins('permissions')                                           
        self.importer = self.create_importer()                                     

        self.config['permissions'] = {                                             
            'file': 777}                                                           

    def tearDown(self):                                                            
        self.teardown_beets()                                                      
        self.unload_plugins()                                                      

    def test_perm(self):                                                           
        self.importer.run()                                                        
        item = self.lib.items().get()                                              
        config_perm = self.config['permissions']['file'].get()                     
        assert check_permissions(item.path, convert_perm(config_perm))             


def suite():                                                                       
    return unittest.TestLoader().loadTestsFromName(__name__)                       


if __name__ == '__main__':                                                         
    unittest.main(defaultTest='suite')

i have the feeling that the function which listens on 'after_write' doesnt get executed on the tests. i put a "print('IM GOT EXECUTED')" in it. and it doesnt get printed on the tests. if i run a normal "beet import myfile" it gets printed. i would be so glad for help.

@sampsyo
Copy link
Member

sampsyo commented Nov 19, 2014

Hmm... I don't see anything wrong immediately. Would you mind adding the tests to this PR, and I'll do some debugging on my end?

@xsteadfastx
Copy link
Contributor Author

added. i have the feeling that it has something to do with the listen('after_write'). maybe i will rewrite the plugin to use listen('import'). maybe that could give me a hint. hey and thanks alot for being so kind to people who try to code something for this aweseome piece of software :)

@xsteadfastx
Copy link
Contributor Author

i changed listen to use "item_copied" instead of "after_write". now everything works even the tests. any thinkable downsides of using item_copied?

@sampsyo
Copy link
Member

sampsyo commented Nov 20, 2014

I'm guessing this is because the test importer is configured not to write files by default. This seems like the right thing in any case, though, because it lets you fix the permissions for new files in your library even if you're not writing tags into files. And it also might be more efficient to only do this on import, trusting the permissions to be fixed from that point on on subequent writes.

Thanks again! ✨ I'll merge this now.

@sampsyo sampsyo merged commit 8784e8d into beetbox:master Nov 20, 2014
sampsyo added a commit that referenced this pull request Nov 20, 2014
Added fix_permissions plugin
sampsyo added a commit that referenced this pull request Nov 20, 2014
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.

2 participants