-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
DB: drop before import & handle errors #1343
base: main
Are you sure you want to change the base?
Conversation
This change is way too big I feel, definitely too big to be one single commit. Imho, we want to fix the importer separately. The whole editing cards and everything has been working fine for years and years on end, I'd rather not touch that without very good reason. It would be better to just start with 1 commit to drop the DB before importing an export, especially because that code wouldn't touch any of the rest of the DB handling, it would live solely in the importing code. |
Sure. That's easy to separate. I'll make a separate PR for that then. But you also wanted to stop silently ignoring errors IIRC. Which is what the rest of the PR does. And that requires a lot of changes, though they are all pretty much the same. |
I guess we misunderstood each other there, I specifically meant that Catima silently ignores errors during importing (conflicts with other cards being the primary issue). The importing is where stuff is in really bad shape, the general DB handling seems... not pretty but functional. I want Catima to handle imports well: detect conflicts and tell the user. However, until that time, wiping the DB before starting the import is the easier fix. |
I get that. The problem is that importing uses the same functions as the rest of the app. I don't think you want the complexity of having an API with and one without errors. But I can change it so that the rest of the code catches the errors but just ignores errors like before. And I can do it in more commits, one function at a time. Just let me know how you'd like to proceed. Will start with a PR just for the DB drop + warning message. |
Agreed. I did write a script to merge two ZIP exports: https://github.com/obfusk/catimerge |
DBException
classDBException
in DB functionsDBException
instead of mostly ignoring return valuesAssert.fail()
: add error messages?#1333 should be rebased on this.
Nice to have