-
Notifications
You must be signed in to change notification settings - Fork 32
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
TextGrid.tierDict can be modified, corrupting the TextGrid #39
Comments
I think full immutability would be pretty cool but I'm not sure how easy it would be to do. I might look into that. If done, then a full set of setter and getter methods will be necessary. Regardless, I like the idea of moving to It might be a while before I can focus my attention on this, but I will sleep on it! |
In hindsight, not very easy at all in Python. You basically have to make a new data structure. I think just marking it as protected and adding a getter makes more sense. But I think just removing the list and relying on the keyset of the
Totally! Actually, as of Python 3.6, all One downside of
Of course, makes sense! I just wanted to post issues as I found them, so I didn't forget! If you're open to it, I might send pull requests later down the road. I'm working on a thesis right now, so it may be some time. |
Sorry for taking so long. I've got a PR up. I decided that for now I'd like to keep the same interface but I resolved the issue where tierNameList can diverge from tierDict. I did this by making tierDict an OrderedDict and making tierNameList a property.
I tried to replicate this in a test but couldn't. Was the issue maybe resolved in Python 3.6? https://docs.python.org/3/library/collections.html#collections.OrderedDict: Please let me know if you have any feedback. |
No worries! Life happens, I know how it goes.
Fascinating. I didn't know they had fixed this behavior. That's really nice! Thanks for making the fixes. I'll take a look at the PR as soon as I can, but if I take too long, obviously don't wait on me :) |
I was a bit on the fence at first but actually I think its a nice change so I added it to my PR as well. |
I merged my PR in prep for a release. But if you have any comments or concerns, please share! I can open a follow-up PR. |
I created a follow-up PR that also makes If you have any feedback, please share! |
Thanks for this! I'll check it out |
I'll close this for now but if there are any related issues, feel free to reopen! |
As far as I can tell from your examples, the canonical way to access a
TextGridTier
from aTextGrid
is to access the internaltierDict
(Example).However, because
tierDict
is mutable, one can end up adding a newTextGridTier
to thistierDict
, e.g.,If this happens, the
TextGrid
will essentially be in an Illegal State, becauseTextGrid.tierNameList
will be missing the new tier. This obviously breaks functions likeTextGridTier#replaceTier
, among other things.I think there are at least two possible solutions:
getTier
method and renametierDict
to_tierDict
to indicate that it is a protected member that shouldn't be alteredtierDict
to anOrderedDict
and removetierNameList
entirely. You can then just usetierDict.keys()
in place oftierNameList
I think it'd probably be best to implement both solutions, particularly because it's dangerous to have two parallel data structures that you need to keep in sync. However, simply implementing number
1
alone should solve the problem quickly and easily.I'm also happy to submit a PR for this if you'd like.
Thanks again for all your work!
The text was updated successfully, but these errors were encountered: