Allow indices and prevent-feeding to work together.

[joanise]

This is not optimized yet, more a proof of concept. Consider this a draft PR, but I'd like feedback on it now anyway please.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (daf33b8) 92.46% compared to head (8363cfa) 92.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   92.46%   92.48%   +0.01%     
==========================================
  Files          16       16              
  Lines        2284     2290       +6     
  Branches      515      516       +1     
==========================================
+ Hits         2112     2118       +6     
  Misses         98       98              
  Partials       74       74              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joanise]

Calling strip_index_notation() for each character being processed is obviously inefficient, but I'd hoping to get feedback on the concept, and if the concept is OK I can optimize.

[roedoejet]

Looks reasonable to me! Tbh I might also be fine with just raising an exception if someone has indices and prevent-feeding=True - this might be a YAGNI. But the implementation looks good - I would be a bit worried about the performance I guess.

[joanise]

No, we can't make this incompatible, I'm about to use if to fix kwk case preservation!

Performance: I did some tests yesterday, and I'm no longer worried: strangely enough, using a cache to make sure I process each rule's output only once made 0 difference on the performance! I guess it's trivial compared to the cost of applying all the rules in the first place. It's only done on the final output of the rules that were actually used, so it kinda makes sense, it's a trivial blip at the end of the pipeline.

[joanise]

Turning this back into a non-draft PR: with the tests I did yesterday, I now think this is ready to merge as is.

[joanise]

never mind, this breaks other stuff, it needs more work...

[joanise]

I think the problem I noticed is just due to #283 and this PR is OK as is, but I'll wait until 283 is fixed before I finalize this PR.

[joanise]

I need this PR to finish my kwk work now, so I've rebased it and improved the unit tests a bit. In my opinion this is ready to merge.

[roedoejet]

Looks good to me @joanise thanks! I noticed that we had the functionality of strip_index_notation in a few places, so I refactored it out everywhere I could find it and just add the commit to this PR, hope you don't mind!

[joanise]

Looks good to me @joanise thanks! I noticed that we had the functionality of strip_index_notation in a few places, so I refactored it out everywhere I could find it and just add the commit to this PR, hope you don't mind!

I think that's an excellent idea, thanks! I just think it belongs in g2p.mappings.utils, alongside things like normaqlize() and unicode_escapes(), instead of being all alone in a new g2p.utils.