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

Improve error messages in case of sqlite errors #3005

Merged
merged 3 commits into from
Aug 15, 2018

Conversation

vvvrrooomm
Copy link
Contributor

@vvvrrooomm vvvrrooomm commented Aug 15, 2018

when beets is started and the database file is on e.g. a volume that is full beets quits with the message "database file could not be opened {path to file}". The original error message coming from sqlite is lost. the proposed patch attaches the original error as 'reason'. in the case of disk full it reads now "database file {path to file} could not be opened, reason: database or disk is full"

I have no idea how to create a test for it, because the test case either depends on a not-writeable database file or on a mock of sqlite

@sampsyo
Copy link
Member

sampsyo commented Aug 15, 2018

Hello! Can you please write something about what you changed and why? Some examples would be extremely useful.

@vvvrrooomm
Copy link
Contributor Author

vvvrrooomm commented Aug 15, 2018

[comments edited]
Also what it the problem with travis ci? I don't understand the error message

@sampsyo
Copy link
Member

sampsyo commented Aug 15, 2018

That helps a lot; thanks!!

Travis is complaining about a line that’s too long and needs to be wrapped:
https://travis-ci.org/beetbox/beets/jobs/416552969#L1103

For grammar’s sake, can we please change the message to database file {} cannot be opened: {}, without the , reason part? That bit seems unnecessary and makes the style seem somewhat strange.

Could you also please include a changelog entry?

@vvvrrooomm
Copy link
Contributor Author

Ah, got it!. yeah I will fix the text and the ci error

@sampsyo
Copy link
Member

sampsyo commented Aug 15, 2018

Looks great; thanks.

@sampsyo sampsyo merged commit b79b759 into beetbox:master Aug 15, 2018
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