wiremod / advdupe2

Advanced Duplicator 2
http://wiremod.com
Apache License 2.0
90 stars 60 forks source link

Attempts to fix issues #389 #399

Closed dvdvideo1234 closed 1 year ago

dvdvideo1234 commented 2 years ago

This will do nicely, @thegrb93

thegrb93 commented 2 years ago

Please revert all the var name changes and parentheses added to if statements and i'll review

dvdvideo1234 commented 2 years ago

Hello, @thegrb93. for https://github.com/wiremod/advdupe2/issues/389

Thanks! Done and reverted! Do you prefer if then than if() then ?

thegrb93 commented 2 years ago

Yes, please undo those.

dvdvideo1234 commented 2 years ago

Yes, please undo those.

I will remove the main () on every IF statement tomorrow assuming you prefer the if then notation

thegrb93 commented 2 years ago

Only ones you added please

thegrb93 commented 2 years ago

Nobody wants to review 300 lines

thegrb93 commented 2 years ago

You can put the cleanup changes in another PR if you want.

dvdvideo1234 commented 2 years ago

@thegrb93

You can put the cleanup changes in another PR if you want.

Done ;)

thegrb93 commented 2 years ago

This doesn't fix #374 so I'm removing that from the title.

thegrb93 commented 2 years ago

I don't like the checks this adds that #374 should be responsible for. This is decent cleanup, but #374 should be the one that does the dupe sanitation.

thegrb93 commented 2 years ago

It looks like the current version of CheckValidDupe already checks headent.Z now https://github.com/wiremod/advdupe2/blob/41023f7e7fd0f209281334bb28d01e32f6ef67b3/lua/advdupe2/sh_codec.lua#L421

dvdvideo1234 commented 2 years ago

This is just for checks in the tool for spawning stuff defaults Z to zero. The user had trouble with pasting an old dupe where such value did not exist ;) I've already checked Edit by maintainers, so you are able you commit stuff yourself ;)

thegrb93 commented 2 years ago

I'll make a couple changes in a while

dvdvideo1234 commented 1 year ago

@thegrb93

Are we gonna work on this too ? I need it for my friend's server.

wrefgtzweve commented 1 year ago

This is just for checks in the tool for spawning stuff defaults Z to zero. The user had trouble with pasting an old dupe where such value did not exist ;) I've already checked Edit by maintainers, so you are able you commit stuff yourself ;)

Yeah we're still getting errors for that multiple times per week, it'd be cool if this pr could be merged.

thegrb93 commented 1 year ago

I'm not reviewing style changes with code changes. Fix the bug and do your style changes in separate PRs please.

dvdvideo1234 commented 1 year ago

@thegrb93 So can I just delete this commit https://github.com/wiremod/advdupe2/pull/399/commits/c743261eee8763592c569ccf5f561dddbddf0d3d by force pushing or should I make new PR ?

thegrb93 commented 1 year ago

I don't like any of the code changes mixed with the style changes. That's why its been open for so long.

thegrb93 commented 1 year ago

Read the comments at the top of the page. Putting code changes and lots of style changes together makes verifying code consistency too hard to bother with.

dvdvideo1234 commented 1 year ago

I'll just make another PR tomorrow