CensoredUsername / unrpyc

A ren'py script decompiler
Other
821 stars 149 forks source link

Dev f-strings #207

Closed madeddy closed 3 months ago

madeddy commented 3 months ago

Port to f-string formating

CensoredUsername commented 3 months ago

Ah, minimize/codegen probably don't support f-strings yet as those didn't exist when they were created. I'll take a look.

madeddy commented 3 months ago

Well it was to expect. The only test i forgot to run uncovering the bugs, is the icing on the cake.

Ah, minimize/codegen probably don't support f-strings yet as those didn't exist when they were created. I'll take a look.

Thanks a lot! I expected its something in the injector files, as unrpyc worked in all tests, but i could not find the real source.

CensoredUsername commented 3 months ago

I just started working on this, and it is surprisingly nontrivial (until python 3.12 there's a lot of weird edge cases around f-string generation relating to reusing quote symbols). Might take me a bit to have the time to work on this, I need to figure out a way to actually handle this properly.

madeddy commented 3 months ago

I had also a dilemma with this quotes and forbidden bslash. To get on a uniform usage of the quotes i put all f-strings in double quotes and everything inside in single q. After like 200 i did first tests and had to find Ren'py couldn't stomach them. Just because i overlooked that the inner single quotes are written out with the strings. Naturally i had to reverse all.

I am glad they figured this in py3.12 out and we have more usage freedom.

Might take me a bit to have the time to work on this

No prob at all. To have f-string is imo nice to have finally, but for now the app works. Also i didn't notice till the checks fail, because i compiled all the time (repeat use of command) with option -r. So without minimization it runs so far.

CensoredUsername commented 3 months ago

Aight, I think I've handled this properly on dev now. Unfortunately github is a bit dumb and I literally cannot rerun tests with changes on the target branch sorted out, but it all seems to work locally.

I'll review the actual pull tomorrow likely.

madeddy commented 3 months ago

I think I've handled this properly on dev now. I'll review the actual pull tomorrow likely.

Oh, good. I expected it not so fast. 👍🏻

cannot rerun tests

I think a different workflow config is needed to get the rerun button or other yaml options to trigger this.: re-running-workflows-and-jobs events-that-trigger-workflows#check_run

Edit: Somehow updating my forks dev branch rebased also my "f-string" pull req branch. What a mess!

madeddy commented 3 months ago

Hehe, ok. I wanted to ask a few weeks ago if you prefer them changed or not. Simply forgot about it. Going to add the relating changes later.

CensoredUsername commented 3 months ago

Aight, thanks!