Split tag reading and tag handling logic #922
Conversation
|
I have also created a small example in a personl branch, showing how the tags can now be handled differently depending on the VERSION header, now that tag reading and handling is split into two steps. |
|
Apologies for the long wait on any feedback, I'm only just now starting to have some time for these again. The code on the surface looks okay. I'll have to take a closer look at it and test it more thorougly, but while doing so I noticed that there is some trailing whitespace being introduced by the commit. Normally we have a CI job for this, but I guess I had to click some button before 30 days had passed... In summary: @TheNailDev can you either sync your fork+this PR (I think a rebase will show me the button again?) or just (force-)push the trailing whitespace removal? I think that'll show me the button again. I think the code is fine, but I'll have to recheck it anyway once the whitespace stuff is fixed. |
TheNailDev
force-pushed
the
tag-dictionary
branch
from
c2dc528 to
22724ef
Compare
|
I just rebased the branch on the current master. Hopefully the CI job will work now. |
|
Hmm, I still don't have don't have a button for it. I'll just do it the old-fashioned way either today or next weekend and fix any whitespace issues myself. Guess github is just being weird. |
There was a problem hiding this comment.
Finally had time to take a good look at this. Overall I agree with this PR, but I found a bug when it comes to the handling of duplicate identifiers, see the comment specific about that. The exception appears to not be caught at all, and the duplicate check doesn't differentiate between USDX tags and other/custom tags.
These need to be addressed before it can be merged.
(don't worry about the whitespace thing I mentioned earlier. I've figured out by now that the check doesn't run for PRs from external repositories at all. It's purely cosmetic and easy for me to fix once the rest of the code is good)
Once the mentioned bug/regression is fixed, this PR is a great improvement on the tag loading. I did look at your linked branch a bit as well and I like what I'm seeing there (but obviously requires this one to go in first).
The error handling for duplicate errors tried to catch the TListError from the wrong unit, which lead to songs with duplicate tags not being loaded at all. Now the logic explicitly catches the TListError from the fgl unit.
According to the format spec, specific header-tags are allowed to be specified multiple times in a song txt file (format >= 1.1.0). The previous tag reading implementation ignored all duplicate tags. Some unsupported custom-tags might already be specified multiple times. As such, the new implementation will not generally ignore duplicated tags, but instead handles deduplication per tag. Thus, duplicated custom-tags are now (again) supported and support for multi-valued headers can be implemented more easily in the future.
barbeque-squared
dismissed
their stale review
all issues have been addressed and whitespace has also been fixed
Hello there,
I saw the comment in #789 from @barbeque-squared about how the current tag reading and evaluation in USong.pas should be changed to allow future extensions, especially with regard to handling different txt versions in the future.
Based of the comment I changed the
ReadTXTHeaderfunction to first read all tags into aTFPGMapand afterward evaluate the tags from the map.I hope this can be a good starting point to allow different tag handling logic depending on the txt version.