Add support for --hex-blob option in mysqldump#71
Add support for --hex-blob option in mysqldump#71vdechef wants to merge 26 commits intomysql2sqlite:masterfrom
Conversation
There was a problem hiding this comment.
Thanks for the PR. It look good to me. Please try to address my comments or discuss them and then I'll merge it 😉. Don't forget also about the following I mentioned in the #69 discussion:
In that case we'd need some "unit" tests (I still couldn't find the time to consolidate and make them work - as commit policy), then I'd add a comment like # as of 2020-03-21 mysqldump --hex-blob is the sole way to get hexadecimal syntax in the whole INSERT/REPLACE output except for substrings of e.g. base64 etc. and last we also need to test it with more awk versions (e.g. macOS awk, FreeBSD awk, GNU awk).
OK, I will fix this later this week. I will also try to improve unit tests. |
|
@vdechef take your time (mine is also a "scarcity"). |
Signed-off-by: vdechef <v.dechef@gmail.com>
|
I think I'm done.
|
dumblob
left a comment
There was a problem hiding this comment.
Thanks! That's a lot of work. Please address my comments (I didn't finish the review for lack of time, but I'll finish it later - hopefully during the next 10 days or so).
Signed-off-by: vdechef <v.dechef@gmail.com>
|
So I just changed the unit tests to match what you wanted. I also took one of the examples you had written, just to be sure that it would work with the helpers functions I added. And thanks to this I realized that the BIT type default values needed hexstring conversion. So I wrote a function for this and it seems like it is fixed now. |
|
While testing a Node.js replacement for mysqldump, I realized that the regex used to handle @dumblob I know you don't have much time, but do you think this PR has a chance to be accepted someday ? |
It's fine, the difference is negligible.
I'm sorry I'm really out of time now, but skimming the PR it looks very good - it's a lot of hard work and I'm glad you devoted so much time and effort to this. To answer your question - yes, I definitely want to merge this PR! One minor nit (I repeat I didn't read the changes thoroughly, so there might be other things once I'll find some more time) would be my preference to not "rename" utilities (with "rename" I'm referring to storing their names in variables and subsequently using these variables). Any good reasons for that? If we really needed to "shadow" a real utility (e.g. to increase cross-platform support or speed or whatever), then I'd definitely rather introduce a differently named function as a wrapper with an easy interface (e.g. fixed number of args) to make the need explicit. |
Nice, I will continue to improve it when needed.
I totally agree. I just tried to mimic the style of existing code, but I didn't feel comfortable with this because the args of those utilities prevent their replacement without code refactoring. |
|
Hi @dumblob, |
Add unit tests
|
I added a few more test cases.
Honestly I don't have enough energy to investigate this |
Add unit tests
|
I just realized that the previous points were invalid:
So I added a few more tests and pushed the code. For the remaining test cases, I don't understand what they are supposed to test so I cannot implement them. |
|
@vdechef this is wonderful! Many thanks! I am indeed much less active during the last years but I really appreciate all the effort you have put into this. Now we are considering moving everything under the organization https://github.com/mysql2sqlite - feel free to express yourself if you find this idea valuable and whether you would wish to become a member (for reviews etc.). |
Fix for https://github.com/dumblob/mysql2sqlite/issues/69
It should also solve https://github.com/dumblob/mysql2sqlite/issues/54