CensoredUsername / unrpyc

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

Improvment propasals #190

Closed madeddy closed 4 months ago

madeddy commented 4 months ago

Main changes:

I wanted to put this forward before upstream changes again and we get conflicts. 😉 Hopefully you find it useful.

CensoredUsername commented 4 months ago

Heya, I've reviewed the changes and left some comments that I'd like to see addressed if I were to merge this. There's a lot of codingstyle-related things in there which I'd usually fix myself before merging single-off contributions, but I figure this ain't the last I'll be seeing of you so it's probably worth typing them out for in the future so things can be merged directly.

In general, I like to work with more, smaller pull requests / commits if possible. There's some interlinked changes in these commits that make it hard to review individual ones, and there's a bunch of stuff I'd merge in a heartbeat, and a few items I'm not sure about. Now on patchsets like this one, that are built on top of each other, you can make that easy by just making a pull request for the first logical commit (which fully handles one specific issue / issue class if possible), and then continuing your work on that branch. After merge / discussions of that one, you can just rebase your work after that on top of whatever resulting merge commit ends up in the repo, resolve any conflicts if necessary, and pull request the next change.

Additionally, this changeset breaks the current test cases, so you should probably address that. I think it also breaks un.rpyc and --try-harder. Now I get that un.rpyc is a bit hard to fix, but I'd like that to be at least mentioned if possible in the description.

madeddy commented 4 months ago

Greetings.

This pull was more of a fast-shot/test balloon before anymore change on the base come in and to see what you think overall. I expected you approve maybe the pathlib part, not sure about the specials extraction, but i was positively surprised.

I had most of this for over two years already and everything on top of each other. Yes, my fault. Very hard to separate this from each other, while solving merge conflicts and picking this over to another repo. This way some stuff got mixed up and unwanted changes slip in. Argh!

There's a lot of codingstyle-related things in there

I have a formatter(yapf, isort) and linter(flake8) running because the are useful IMHO. Mostly. Downside is they change things like two lines between func etc., one between methods and paint many things in red till you address it and i settled on this by now. On this note, maybe you could drop a line or two somewhere about the style you prefer for contributions.

this ain't the last I'll be seeing of you so it's probably worth typing them out for in the future

Quit alright. I value good critique, try to learn and my nose tells me you're not inexperienced. My area in IT is more the tech-side and coding is autodidact and mostly for hobby.

I like to work with more, smaller pull requests

Yeah, i add mostly too much into one commit. I see the points in this.

... planning to move... to f-strings ...anyway!

A relief. I love them and hate the old two ways. So no trouble for you, if you're interested, i can do all f-strings after the work on this here.


So, about the issues with this. What do you prefer? Doing changes on the pull with a dozen additional commits seems no good idea. Better i pull it after some discussions back and redo it in smaller commits. Or? Also in more diff pulls?

I address then the review posts one by one.

CensoredUsername commented 4 months ago

So no trouble for you, if you're interested, i can do all f-strings after the work on this here.

Go ahead. I'd exempt any file that's PY2/3 compatible already (codegen.py, magic.py, minimize.py) but the rest can be ported.

So, about the issues with this. What do you prefer? Doing changes on the pull with a dozen additional commits seems no good idea. Better i pull it after some discussions back and redo it in smaller commits. Or? Also in more diff pulls?

What I'd do is address the commits first to get the branch in the state that you want. I'd then switch to a different branch and incorporate the final state of the changes in a more reasonable order. I'd do something like this first:

Just post them as pulls one by one, that way we can easily see if nothing breaks tests, and if anything is needed to fix un.rypc I can step in there.

madeddy commented 4 months ago

OK. Will do. I need to address some things in discussion but basically we're done here and i close it.