Added an --xml-dir option to pbp-meta-gen #36
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
My team required this feature. I made this work by reworking the abstract class. If you do not specify ----xml-dir you the behavior will default to prior build where xml log path is assumed to be the same as the audio directory.
I had some development artifacts from testing earlier so I had to make some edits to the xml-dir branch that wont be required in the main.
|
I forgot about the tests. Ill take care of those shortly. |
Attempting to fix two tests.
I had tests fail because I was borrowing the same abstract class. Here I just made a second class that was compatible with sound-trap. This way the original implementation remains unchanged allowing the tests top pass.
I had forgot to change the to new abstract class argument.
|
What a terrific contribution. The option and its behavior make good sense to me. |
I modified the try block in the abstract to be consistent.
|
Thanks Danelle, glad you like it. In the end I had to create a new abstract class specifically for SOUNDTRAP because I couldn't impose an xml-dir required or not on the other devices. Still working on fixing this last build error. |
Removed the file:// string from xml path since it is more simple than that.
Ran a ruff on the files.
The main method implementation wasn't changed until now. Since I didn't technically need it it was only the linters that caught it.
I forgot to run a ruff on the modified files.
|
Thanks Danelle! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have tested all scenarios I can think of. Linux, windows and all possible command configurations where --xml-dir is not specified and where it is. In all cases it passes the tests I imposed on it. If I move around the xml logs it works, if it cant find them it will tell you. As far as the abstract class is concerned, the xml-dir is now required but the scripts will provide the variable as the same as the audio directory unless the user specifies otherwise by --xml-dir.