Skip to content
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

Attempts to fix issues #389 #399

Closed
wants to merge 8 commits into from
Closed

Conversation

dvdvideo1234
Copy link
Contributor

No description provided.

Added: Dedicated angle function `GetDupeAngle`
Improved: Long lines poor visuals
@dvdvideo1234
Copy link
Contributor Author

This will do nicely, @thegrb93

@thegrb93
Copy link
Contributor

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

Added: Spaces between logical and math operations
Added: Spaces before `then` statement
@dvdvideo1234
Copy link
Contributor Author

dvdvideo1234 commented May 26, 2022

Hello, @thegrb93. for #389

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

@thegrb93
Copy link
Contributor

Yes, please undo those.

@dvdvideo1234
Copy link
Contributor Author

dvdvideo1234 commented May 26, 2022

Yes, please undo those.

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

@thegrb93
Copy link
Contributor

Only ones you added please

@thegrb93
Copy link
Contributor

Nobody wants to review 300 lines

@thegrb93
Copy link
Contributor

thegrb93 commented May 26, 2022

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

@dvdvideo1234
Copy link
Contributor Author

@thegrb93

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

Done ;)

@thegrb93
Copy link
Contributor

thegrb93 commented Jun 1, 2022

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

@thegrb93 thegrb93 changed the title Attempts to fix issues #389 and #374 Attempts to fix issues #389 Jun 1, 2022
@thegrb93
Copy link
Contributor

thegrb93 commented Jun 1, 2022

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
Copy link
Contributor

thegrb93 commented Jun 1, 2022

It looks like the current version of CheckValidDupe already checks headent.Z now

function AdvDupe2.CheckValidDupe(dupe, info)

@dvdvideo1234
Copy link
Contributor Author

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
Copy link
Contributor

thegrb93 commented Jun 2, 2022

I'll make a couple changes in a while

@dvdvideo1234
Copy link
Contributor Author

@thegrb93

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

@wrefgtzweve
Copy link
Member

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
Copy link
Contributor

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

@thegrb93 thegrb93 closed this May 12, 2023
@dvdvideo1234
Copy link
Contributor Author

@thegrb93 So can I just delete this commit c743261 by force pushing or should I make new PR ?

@thegrb93
Copy link
Contributor

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

@thegrb93
Copy link
Contributor

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
Copy link
Contributor Author

I'll just make another PR tomorrow

@dvdvideo1234 dvdvideo1234 deleted the fix_389 branch May 15, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants