MTG / acousticbrainz-client

A client to upload data to an acousticbrainz server
GNU General Public License v3.0
29 stars 22 forks source link

AB-293: Proper exit status for files #46

Closed rsh7 closed 6 years ago

rsh7 commented 6 years ago

I am first reading the reason column from the filelog and printing this reason before moving on to the next file.

alastair commented 6 years ago

Thanks for your contribution. Unfortunately we can't accept this patch as it currently is. We want the number of dependencies of this software to be as low as possible. In fact, we include requests in the package and don't require any other external software. For this reason we don't want to use pandas - it would require our users to install this software separately in order to be able to use the tool.

Having said that, there is no need for pandas here - we should be using the standard sqlite library to access the file log.

The _update_progress method is a utility used only for printing messages to the screen. There should be no logic in this method. In fact, I can't even see how you get the filepath variable in this method.

As I mentioned in the previous PR, we should read the status from the database. A new method get_processed_status could return GREEN, ":) done" if the file was previously successfully processed (that is, the reason column in the filelog table is null), or RED, ":( nombid" if there is a reason in the column. It would also have to return something based on the extractor reason. Note that the printed message in this case just says :( extract - this is so that it fits within the [ ] brackets, which only have . You'll also have to return the colour to pass to _update_status

rsh7 commented 6 years ago

On reprocessing files, get_processed_method is reading the value of filelog.reason for the particular filepath from database and returning the status based on the values.

rsh7 commented 6 years ago

Problem Statement If we submit audio files for the first time which has no MBID, the exit status comes out to be [:( nombid ] After that any number of times if we submit the same files and without any MBID, the exit status changes to [:) done ], which is not expected.

Solution Exit status should be same on repeated trials of submissions of same set of files. Now, a method get_status is getting the status from the filelog table by reading the reason column for every music file one by one. Now, the colour and status which are getting passed to _update_status, are GREEN, ":) done" if the file was previously successfully processed, RED, ":( extract" for extractor reason and RED, ":( nombid" if there is any other reason in the column. Now, the proper exit status will be displayed for every acousticbrainz-client submissions of music files.

alastair commented 6 years ago

Thanks for the patch! I've merged your changes but also made some other updates. You can see them in https://github.com/MTG/acousticbrainz-client/commit/92078ecd583e9103bd8b83e02e03a7a9fc08d479

Instead of a separate is_processed and get_processed_status method, I made a single method get_processed_status which checks and returns the value. As @Freso noted, there are many types of error status. Your updated change added nombid, but there is also json, added here: https://github.com/MTG/acousticbrainz-client/blob/92078ecd583e9103bd8b83e02e03a7a9fc08d479/abz/acousticbrainz.py#L151 By getting the status directly from the database we don't need to have the if/elsif block for all cases. If we add another message in the future we won't have to add another check.