-
Notifications
You must be signed in to change notification settings - Fork 2
V7 Support #135
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
base: main
Are you sure you want to change the base?
V7 Support #135
Conversation
|
Oh wow, this is quite a surprise! I will sit down and take a look at all of this over the weekend sometime. Thank you very much for this! So basically this now has a way to read but not write V7 files, and it does this by using these generated |
|
Yep. The .proto files did all the heavy lifting. From there, it's just a matter of using a library to parse (and also build) into a JSON format. The annoying part is mapping the data both ways |
|
Ok, I finally took a look at this, sorry for taking so long. I pushed a few minor changes into the main branch, and you'll need to pull latest into your PR branch to resolve merge conflicts on the package files. I have to requested changes though:
Also a question: This project is used in LyricConverter (http://lyricconverter.net and code at https://github.com/ChrisMBarr/LyricConverter) - Will your changes here also work in a web browser context? Not just in Node? |
|
Unit tests: I can write that, but I currently don't have access to a good V7 file for testing. I also run Linux, so it's quite a hassle to find a machine to run the software and create one. If you have a good file, that would be amazing. Code complexity: Honestly i'm not so sure what to do about that. The types that were generated from the .proto files makes it so most fields are nullable. Meanwhile, our mapped type/model are more strongly typed. That's why most of it requires the null coalesce statements. Unless we change our model or the .proto type definitions, I'm not sure what i can do here. Browser: Good question... Looking at the code again, i believe most of it should do. The question mark is in the |
|
Ok, that all makes sense. I have a Mac and can use the free trial will try to generate some simple Pro7 files for you. I'll make a simple one, and then try importing a pro6 file and saving that out as pro7. Ok, makes sense about the code complexity stuff, just wanted to make sure. We can probably just add some lint ignore statements and explain there's just not a great way to simplify since they truly are all possibly null. Another parser I made was for SongShowPlus files, works in the browser. It has a check for let fileBuffer: ArrayBuffer;
//The Node Buffer is not available in web browsers, so we want to make sure it exists before attempting to use it
if (Buffer?.isBuffer(rawBuffer)) {
//Convert Node Buffer to an array buffer
//https://stackoverflow.com/a/71211814/79677
fileBuffer = rawBuffer.buffer.slice(
rawBuffer.byteOffset,
rawBuffer.byteOffset + rawBuffer.byteLength
) as ArrayBuffer;
} else {
fileBuffer = rawBuffer as ArrayBuffer;
}You can see the code for that here if you want to compare: https://github.com/ChrisMBarr/SongShowPlus-parser/blob/main/src/index.ts |
d24af94 to
a5ad66f
Compare
|
Thanks! I disabled the code complexity rule on the file, rebased to main & also updated the Buffer call for compatibility with browser. I used TextEncoder instead since it's quite straightforward. |
|
Wow, they now really push you do start a 14 day trial with a credit card, but they do still have a downloadable demo version thankfully. Ugh! Ok, so I got a few simple songs imported and I took some screenshots of the UI for each one so you can see what it contains since you aren't able to run the software The one for the "Feature Test" is for sure not as important, but it's a file I used for the V6 parser. The lyrics are for sure more important. |
|
Thanks @ChrisMBarr! Sorry for the delay, I just finally have the chance to work on this 😓 |
|
Hey nice, that's great! Sorry the workflow didn't run automatically when you submitted this, I've made the settings change so that it will now. Looks like there are a few areas of code not covered by unit tests. Could you add those, or if not possible/reasonable to test then add the |
Attempt to add V7 support to the project. Thanks to the pointers in #1
This implementation:
Notes:
Let me know what you think!