🚧 Ignore escaped characters

[akaszynski]

This take a stab at ignoring escaped characters. It's a basic (and probably inefficient) implementation, so if there's a better way to get this done, please let me know.

Also, the escape dictionary should be modifiable by the user.

[larsoner]

On a real code base this PR takes my run from 1.097 sec to 1.508 sec. If I change your function to:

    for key, rep in find_dict.items():
        text = text.replace(key, rep)
    return text

it only increases to 1.161 sec. So maybe regex is less than ideal here.

Can you check on PyVista and see if you observe the same?

[akaszynski]

Same problem here. I've noticed that regex is almost always slower than the built-in replace.

[akaszynski]

We're now using replace. As for making the substitution dictionary customizable, would you prefer to have a file containing the key/value pairs, or a string that they read in?

[larsoner]

As for making the substitution dictionary customizable, would you prefer to have a file containing the key/value pairs, or a string that they read in?

I find putting characters like \n in the command line to be annoying (usually takes me multiple tries to get it right), so I would lean toward a file

[akaszynski]

Agreed, I was encountering that issue as well when testing it out. I'm thinking simple key value pairs in a csv, where the pairs are separated by line breaks.

"\n", " "
"\'", "'"

[larsoner]

Might make more sense to match the dictionary format

thing->replacement

[akaszynski]

As we’re going to have to use spaces, are quotes permitted in your dictionary format?

"\n"->" "
"\'"->"'"

[larsoner]

I would just make it that anything to the left of -> is the substring to look for, and anything to the right is the replacement. So:

\n-> 
\'->

[akaszynski]

Should be good to review now.

[larsoner]

Looks good. One last thing: we probably want to provide and use a default file, just like we do for the dictionary. Maybe it should just contain \n-> to start.

[akaszynski]

Looks good. One last thing: we probably want to provide and use a default file, just like we do for the dictionary. Maybe it should just contain \n-> to start.

Perhaps \'->' as well.

[larsoner]

Fine with me

[peternewman]

I'm a bit unclear from the help detail what this is for, is it to "fix up" the dictionary to deal with it matching escape sequences? To actually do sed type runs on my codebase or something else?

[peternewman]

Is this linked to #233 ?

[larsoner]

Also in the related PR #174 it was noted that the write-changes flag was problematic when dealing with escapes, if we come back to this we should make sure that write-changes works even with these substitutions

[bl-ue]

Why the close? I see this is quite old 🤔

[akaszynski]

Why the close? I see this is quite old thinking

Just cleaning up old PRs. I'd like to work on this, but this project hasn't been updated in quite some time (last release 6 months ago).

[peternewman]

Hi @akaszynski ,

Despite the lack of releases, coding is still happening (although mostly in the dictionary), also see #1923:
v2.0.0...master

You didn't seem to have respond to the review comments myself and @larsoner had left if you were expecting it to have been merged.

[akaszynski]

You didn't seem to have respond to the review comments myself and @larsoner had left if you were expecting it to have been merged.

True, there's still work to be done. I'll work on this.