New export command (issue #435) #2510

SpirosChadoulos · 2017-04-13T10:09:18Z

Could you help me with the errors that I am getting?

mention-bot · 2017-04-13T10:09:23Z

@SpirosChadoulos, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @pkess and @geigerzaehler to be potential reviewers.

sampsyo · 2017-04-13T11:15:19Z

beets/ui/commands.py

@@ -1450,7 +1450,7 @@ def modify_func(lib, opts, args):

 # move: Move/copy files to the library or a new base directory.

-def move_items(lib, dest, query, copy, album, pretend, confirm=False):
+def move_items(lib, dest, query, copy, album, pretend, confirm=False, export):


This:

ERROR: Failure: SyntaxError (non-default argument follows default argument (commands.py, line 1453))

Means that export must either come before confirm or have a default value.

I still have an error at line 1483 with item

Can you please be a little more specific? I can't quite find the error you're talking about; maybe you can include the error text?

Unresolved reference 'item' less... (Ctrl+F1)
This inspection detects names that should resolve but don't. Due to dynamic dispatch and duck typing, this is possible in a limited but useful number of cases. Top-level and class-level items are supported better than instance items.

sampsyo · 2017-04-13T12:40:27Z

beets/ui/commands.py

-                lambda o: show_path_changes(
-                    [(o.path, o.destination(basedir=dest))]))
+        if export:
+            util.copy(item.path, item.destination(basedir=dest))


Is this the line you're referring to? If so, item is indeed an undefined variable. You will need to assign to it before using it. Did you mean obj? But obj might be an item or an album, so you may need to check which it is.

this is the line I'm refering to util.copy(item.path, item.destination(basedir=dest))

@sampsyo how should I handle item ?

As I mentioned above, that variable is not defined. I suggested that you might have intended to use obj instead.

I used it and I got no errors but appveyor and travis failed..Could you help me with that?

I should only care about the failures, not the errors right?

Errors are important too—that's when an unexpected exception is thrown.

I noticed something weird. AppVeyor and Travis say that I have 4 failures but when I run the command nosetests locally I get 3 failures and when I run the command python setup.py test locally I get 7 failures. How is that possible? Also in the local tests, no matter what I change in my if export statement the failures remain exactly the same.

That's odd! Which test do the two runs disagree on?

To help debug the failures, sometimes it can be helpful to try deleting the code you added and confirming that all the tests pass, as expected. Then you can add changes in one at a time to see when things break.

I deleted all the code I added to this file and the failures are still the same.. That's really weird..

SpirosChadoulos · 2017-04-19T10:18:40Z

@sampsyo I fixed some issues and now my code passes the tests. Is everything okay?

sampsyo · 2017-04-19T23:12:27Z

Yes; this looks great! Thank you for sorting out the various problems! ✨

To make this ready to merge, we'll need at least an update to the docs (to describe the new option) and a changelog entry. It would also be super nice to have a unit test, which shouldn't be too hard—it can be pretty similar to a copy n' paste of one of the existing tests for the move command, but it should check that the item's path in the database doesn't change.

SpirosChadoulos · 2017-04-22T10:07:50Z

Okay I'll do these the following days.

SpirosChadoulos · 2017-04-29T11:56:06Z

Should I copy n' paste the MoveTest class (test_ui.py)??

sampsyo · 2017-04-29T21:32:26Z

Yeah, that seems like a great place to start!

SpirosChadoulos · 2017-04-30T12:31:48Z

@sampsyo I need some help with the errors I am getting because it is the first time I am writting a test for beets

sampsyo · 2017-05-01T19:11:22Z

Cool! Happy to help, but to make this easy, would you mind pasting the specific errors you have questions about and give a little context? Then I can totally help with some answers.

SpirosChadoulos · 2017-05-03T08:55:35Z

All my errors are about no such filed or directory errors.. Maybe I've done something wrong that affects all of them.

sampsyo · 2017-05-03T13:46:40Z

OK! As usual, though, your part of the bargain is to lay out the problem clearly so your collaborators have a hope of being helpful. Specifically:

Copy and paste the error message.
Refer (perhaps link) to the part of the code where you think it's coming from.
Give your theories so far about what might be going wrong.
Ask specific questions about where you're confused.

SpirosChadoulos · 2017-05-03T19:10:23Z

https://travis-ci.org/beetbox/beets/jobs/227346244#L887-L901

Here the error I get is AttributeError: no such field 'destination', but I can't figure out what this means and where this field is.
Also, I did what you told me and I copy n pasted the MoveTest in order to create the ExportTest and I added export=True. I noticed that the test methods that fail in the new class are those that go through the new code I added in commands.py(which you told me was okay). I can't figure out if the test methods need some tweaking or if the main code is the problem and the tests fail because the new feature isn't doing what it is supposed to do.

sampsyo · 2017-05-04T13:19:51Z

Ah, I see—thanks for clarifying!

The problem here is that obj might be an Album object or it might be an Item object. On this line, your code assumes it's an Item—only Items have a destination method. You'll need an if album: condition, just like the one earlier in the method, to iterate over the album's items in album mode.

SpirosChadoulos · 2017-05-04T15:41:39Z

Perfect, that was very helpfull! I will do exactly that.Thanks a lot for the help!!

SpirosChadoulos · 2017-05-04T16:29:52Z

@sampsyo https://travis-ci.org/beetbox/beets/jobs/228798906#L887-L899
TypeError: copy() missing 1 required positional argument: 'dest'..Why is this happening? My copy function has a dest parameter. Didn't I did what you told me above the right way?

sampsyo · 2017-05-04T20:47:04Z

beets/ui/commands.py

+                                   for obj in objs for item in obj.items()])
+            else:
+                util.copy([(obj.path, obj.destination(basedir=dest))
+                                   for obj in objs])


Here, you're just passing one argument—a list, produced by a list comprehension—to the copy function. Maybe you mean something like:

for item in objs: util.copy(item.path, item.destination...)

and then again with for item in obj.items().

https://travis-ci.org/beetbox/beets/jobs/228909615#L898
To which file is it refering to? Didn't I do what you explained right?

sampsyo · 2017-05-08T23:32:46Z

Hmm; yes, that's not showing you the precise traceback. One way to test a theory is to run the test locally and add print statements around where you think the failure might be occurring. That can help you narrow down both where things are going wrong and why.

SpirosChadoulos · 2017-05-09T23:03:09Z

I did some research and some testing and I think that the tests are okay and the problem is the util.copy(item.path, item.destination(basedir=dest)) call, especially the 2 parameters the method gets.I also think that the problem is the destination path (2nd parameter) and not the first one. Any thoughts and suggestions?

sampsyo · 2017-05-09T23:15:35Z

Hmm; what do you think the problem is there? That call looks (to me) like it copies the file from its original location to the path where it needs to go. Seems correct, I guess?

SpirosChadoulos · 2017-05-10T08:53:04Z

But if this is the case why do we get this error? https://travis-ci.org/beetbox/beets/jobs/229424482#L983
Note that this error occurs just when self._move() is executed in our tests and not when the assertions happen.

sampsyo · 2017-05-11T00:56:09Z

Good question. Since the error says this:

beets.util.FilesystemError: No such file or directory while copying /tmp/tmppqnmxzp9/testlibdir/srcfile to /tmp/tmppqnmxzp9/testotherdir/the artist/the album/02 full

that can actually mean either the source or the destination doesn't exist. I suspect it's the destination. Specifically, the destination path comes from here:

self.otherdir = os.path.join(self.temp_dir, b'testotherdir')

but that directory is never created!

The solution is probably to add code to the export functionality to create parent directories. For example, you can see that the move method on Item uses this code:

# Create necessary ancestry for the move.
util.mkdirall(dest)

to make sure that the destination directory will exist.

SpirosChadoulos · 2017-05-11T09:56:33Z

I fixed it!! I think it works fine now. What should I do now in order to merge this pull request?

sampsyo

Yay yay yay! That's great! 🎉 ✨

I have a few code suggestions above, where the two themes are removing redundancy and thinking carefully about the naming of test cases.

There's one last thing we'll need in order to merge: documentation! Can you please add a brief description of the new flag to docs/reference/cli.rst under the move command?

sampsyo · 2017-05-11T11:28:02Z

beets/ui/commands.py

@@ -1479,6 +1480,19 @@ def move_items(lib, dest, query, copy, album, pretend, confirm=False):
            show_path_changes([(obj.path, obj.destination(basedir=dest))
                               for obj in objs])
    else:
+        if export:
+


Could you please clean up this extra blank space, and instead add a brief comment about what's going on here?

sampsyo · 2017-05-11T11:28:25Z

beets/ui/commands.py

+                for obj in objs:
+                    # Create necessary ancestry for the copy.
+                    util.mkdirall(obj.destination(basedir=dest))
+                    util.copy(obj.path, obj.destination(basedir=dest))


Since these two lines are identical to the above, a "helper function" to encapsulate them would help with maintainability.

Do you mean lines 1489&1490 and 1494&1495?? They are not exactly identical, one uses item and the other uses obj. Do you want me to create a helper function that takes as a parameter either item or obj?

Yes, exactly.

sampsyo · 2017-05-11T11:29:16Z

test/test_ui.py

+    #
+    #        self.run_command('--config', cli_config_path,
+    #                      '--config', cli_overwrite_config_path, 'test')
+    #        self.assertEqual(config['anoption'].get(), 'cli overwrite')


This change looks like it's just unrelated noise—would you mind reverting it?

sampsyo · 2017-05-11T11:29:55Z

test/test_ui.py

+        self.i.load()
+        self.assertTrue(b'testlibdir' in self.i.path)
+        self.assertExists(self.i.path)
+        self.assertExists(self.itempath)


Export mode ignores the copy flag, so this test is probably redundant and can be removed. (The album versions with this flag are the same way.)

sampsyo · 2017-05-11T11:31:01Z

test/test_ui.py

+        commands.move_items(self.lib, dest, query, copy, album,
+                            pretend, export)
+
+    def test_move_item(self):


test_move_item no longer really accurately describes what's going on here. Maybe you mean test_export_item? (And move -> export in other test names below?)

sampsyo · 2017-05-11T11:31:49Z

test/test_ui.py

+        self.i.load()
+        self.assertTrue(b'testlibdir' in self.i.path)
+        self.assertExists(self.i.path)
+        self.assertNotExists(self.itempath)


The move flag is also ignored, right? So this test seems similarly redundant.

SpirosChadoulos · 2017-05-12T12:36:40Z

@sampsyo Are all of your change requests covered? Do you want anything else before I write the docs?

sampsyo · 2017-05-13T03:16:50Z

Yes; this all looks great! I think the last missing piece is the docs.

SpirosChadoulos · 2017-05-13T09:37:14Z

Ready to merge?

SpirosChadoulos · 2017-05-15T21:49:47Z

@sampsyo Sorry for tagging you but I thought you might have missed my comment..Are we ready to merge?

sampsyo · 2017-05-16T15:47:53Z

Hi! Sorry, I did see your comment—things look good, and it's on my to-do list to do a complete review and merge.

SpirosChadoulos · 2017-05-16T17:02:25Z

Okay thanks a lot!!

The calls didn't match up with the parameter order.

The old tests were wrong but the incorrectness was hidden by the incorrect parameter passing fixed in the previous commit. Now we actually test that the item's path did not change.

Just one new flag.

New export command (issue #435)

sampsyo · 2017-06-11T16:24:09Z

Sorry I missed the May deadline! I finally had a moment to give this the attention it deserved. I simplified a few things, revised the tests, fixed some parameter-passing issues, and merged. ✨ Thank you for your help!

New export command

4745c26

sampsyo reviewed Apr 13, 2017

View reviewed changes

SpirosChadoulos added 8 commits April 14, 2017 12:19

Put export before confirm

8f3ca12

obj instead of item

d4413a2

Provided default value for export.

a99b7e9

added 1 line before for loop

5361825

deleted else

7018307

deleted else

3e9076b

fixed certain errors

de57602

fixed line length

60318f1

SpirosChadoulos added 2 commits April 30, 2017 15:02

Added a test for the new export feature

bab99f5

ExportTest correction

a881922

Added an if album: statement

90c30d8

sampsyo reviewed May 4, 2017

View reviewed changes

Changes at line 1486

167ae91

SpirosChadoulos added 3 commits May 11, 2017 12:11

Made sure that the destination directory will exist

0dc948d

Minor fixes to move tests

51835e7

Minor Flake fixes

c4ef23d

sampsyo requested changes May 11, 2017

View reviewed changes

SpirosChadoulos added 3 commits May 12, 2017 14:49

Requested changes done

8f62e8b

Fixed bool variable error

169cf59

Minor flake fixes

7c91989

docs

3c852d3

sampsyo added 6 commits June 10, 2017 20:03

Fix some spurious whitespace changes

29d6c27

Remove unnecessary output capture

b25eb87

Simplify implementation of export behavior

2315287

Fix parameter order and binding

714560a

The calls didn't match up with the parameter order.

Correct tests for export mode

730c84e

The old tests were wrong but the incorrectness was hidden by the incorrect parameter passing fixed in the previous commit. Now we actually test that the item's path did not change.

Consolidate export tests into MoveTest

ca4f96e

Just one new flag.

sampsyo merged commit ca4f96e into beetbox:master Jun 11, 2017

sampsyo added a commit that referenced this pull request Jun 11, 2017

Merge pull request #2510 from SpirosChadoulos/master

dfde9ce

New export command (issue #435)

sampsyo added a commit that referenced this pull request Jun 11, 2017

Changelog for #2510 (fixes #435)

f65653a

New export command (issue #435) #2510

New export command (issue #435) #2510

Conversation

SpirosChadoulos commented Apr 13, 2017

mention-bot commented Apr 13, 2017

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

SpirosChadoulos commented Apr 19, 2017

sampsyo commented Apr 19, 2017

SpirosChadoulos commented Apr 22, 2017

SpirosChadoulos commented Apr 29, 2017

sampsyo commented Apr 29, 2017

SpirosChadoulos commented Apr 30, 2017

sampsyo commented May 1, 2017

SpirosChadoulos commented May 3, 2017 • edited Loading

sampsyo commented May 3, 2017

SpirosChadoulos commented May 3, 2017

sampsyo commented May 4, 2017

SpirosChadoulos commented May 4, 2017

SpirosChadoulos commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampsyo commented May 8, 2017

SpirosChadoulos commented May 9, 2017

sampsyo commented May 9, 2017

SpirosChadoulos commented May 10, 2017

sampsyo commented May 11, 2017

SpirosChadoulos commented May 11, 2017

sampsyo left a 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

SpirosChadoulos commented May 12, 2017

sampsyo commented May 13, 2017

SpirosChadoulos commented May 13, 2017

SpirosChadoulos commented May 15, 2017 • edited Loading

sampsyo commented May 16, 2017

SpirosChadoulos commented May 16, 2017

sampsyo commented Jun 11, 2017

SpirosChadoulos commented May 3, 2017 •

edited

Loading

SpirosChadoulos commented May 15, 2017 •

edited

Loading