-
-
Notifications
You must be signed in to change notification settings - Fork 262
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
py3: add new helper: get_replacements_list #2242
Conversation
0596fad
to
2d05477
Compare
Ideally, we should separate this into
|
Merged verison of the two regex patterns according to GPT. Untested.
|
@ultrabug Review plz. |
I didn't add this for I don't need this for |
Hi @lasers sorry for the delay I agree this approach is sane and works good enough. I would have argued that we could have done the replace work within py3 core so that modules didn't have to all implement the same replace loop but hey, let's move forward |
Used regex from spotify example config:
Some findings by testing it with some youtube videos: Generally looks promising, but most difficult part is to iron out good regex, After implementing code, got one qestion. Is there difference between using self.replacements_init vs self._replacements_init in module code? |
That's why we have user-configurable regex now. I tried what I can to consolidate both regexes so the example config would look this clean. Does this issue occur the same with old regexes? If yes, then it's perfect, but the example can be changed later on. This is also why I think it's sensible not to default
Can you explain more? |
An alternative to PRs listed below.
I copied
thresholds
code. Minimal test. Allow users to be more specific.Get placeholders_list first so we can target existing placeholders rather than all placeholders.
I don't know if this PR is the optimal solution, just an alternative.
Code untested on this player.. I tested on
nvidia_smi
. Sorry if you ran into issues.This is just an idea / experiment.
Please test this.
Solves #2234 (comment).
Closes #2234.
Closes #2237.