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

Voice parsing fix for bug #267 #329

Closed
wants to merge 4 commits into from
Closed

Voice parsing fix for bug #267 #329

wants to merge 4 commits into from

Conversation

huispaty
Copy link
Collaborator

Fixes bug #267 for voice parsing from match files

@huispaty huispaty added the bug Something isn't working label Oct 11, 2023
@huispaty huispaty changed the base branch from main to develop October 11, 2023 11:53
@huispaty huispaty removed the request for review from CarlosCancino-Chacon October 11, 2023 11:53
@sildater
Copy link
Member

Thanks for looking into this!

this PR has several problems:

  • the implementation of voice parsing were voices are encoded as score attributes "vXYZ" is not working. Currently all match files of the newest version get all voice attributes set to 1 (the reason it's not none is the code in lines 747-750 in importmatch.py). Please check what strings the number_pattern regex is matching. If the logic is not clear, just ask.
  • it removes prior check for numbers (strings that only contain integers) in the scoreAttributeList. Are you sure this never happens in any match file? Unless you know for certain that the current implementation is unused or your replacement refactors the use case, please do not remove or change used code and cases from the library!
  • minor point but it'd still be nice: some unittest for different versions of match files would be great.

Copy link
Member

@sildater sildater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't work yet. not automatically tested. removes potentially used functionality.

@manoskary
Copy link
Member

doesn't work yet. not automatically tested. removes potentially used functionality.

The tests were not launched because the initial push was made for the main branch and then changed, an additional commit even an empty one, has to be made to launch the tests. I will do that.

Removed a space to launch tests. No code edit was made.
@sildater
Copy link
Member

doesn't work yet. not automatically tested. removes potentially used functionality.

The tests were not launched

Thanks! What I meant is the functionality in the PR is not addressed in a unittest.

@huispaty huispaty added the stale label Oct 23, 2023
@huispaty huispaty closed this Oct 23, 2023
@huispaty huispaty deleted the match_import branch October 23, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants