vendethiel / GladiusEx

13 stars 19 forks source link

WotLK fixes #57

Closed ManneN1 closed 1 year ago

ManneN1 commented 1 year ago

Hello,

I fixed all (?) the bugs related to WotLK.

Noteables:

Note: I have not tested this on BCC or retail. It might not work there, and the PR might therefore need some more tuning to work correct on the other versions of the game.

vendethiel commented 1 year ago

While I really appreciate this PR, I'm not super fond of the reformatting that was applied... If I come to merge this, I'd prefer that the formatting was applied everywhere + it has to be something you can call from the CLI (so I can have a pre-commit hook for that style to be coherent in the future).

ManneN1 commented 1 year ago

While I really appreciate this PR, I'm not super fond of the reformatting that was applied... If I come to merge this, I'd prefer that the formatting was applied everywhere + it has to be something you can call from the CLI (so I can have a pre-commit hook for that style to be coherent in the future).

The formatting in this addon is a mess with or without this PR. It's a mix of tabs, 2-space indent, and 4-space indents everywhere. Additionally line length and other general coding guidelines are also all over the place. This is especially true if you also include the libraries which are used by the addon (thinking mainly of LibCooldownTracker).

Feel free to apply your own formatting coherently in the code.

vendethiel commented 1 year ago

Sorry, I think my intention didn't properly come across. The only part I disagree with is formatting being mixed with the rest of changes, because that makes reviewing more difficult, but it's fine with ?w=1 on github.

The formatting in this addon is a mess with or without this PR. It's a mix of tabs, 2-space indent, and 4-space indents everywhere

And that's why I didn't defend it at all. I thought the second part of my comment made that clear, this is a move in the right direction.

I however very much agree with a need for a cleanup. I'm more than happy to reformat everything, but preferably only once (so all files are changed at once and not later, unless there are actual code changes) and have it automated for future code changes (that's why I said I preferred something I can run at will).

There's absolutely no point to you applying formatting, then me, then you, except for churn and everything being modified every other PR. So let's pick one option, stick to it, and things will be fine.

ManneN1 commented 1 year ago

There's absolutely no point to you applying formatting, then me, then you, except for churn and everything being modified every other PR. So let's pick one option, stick to it, and things will be fine.

I guess I must've been unclear but I thought I made it clear that I recommend for you to set the formatting of all files to what you prefer in order to avoid the situation as described above.

Sorry, I think my intention didn't properly come across. The only part I disagree with is formatting being mixed with the rest of changes, because that makes reviewing more difficult, but it's fine with ?w=1 on github.

That's fine, but I needed to fix the formatting of the files I was working with to even be able to work with it without wanting to pull my hair out. I removed most of it before committing.

vendethiel commented 1 year ago

I guess I must've been unclear but I thought I made it clear that I recommend for you to set the formatting of all files to what you prefer in order to avoid the situation as described above.

The only missing thing is to decide on one formatting style. I'm not very knowledgeable about lua formatters, I'm not sure if the formatting you have here is the result of your IDE or of some other program.

ManneN1 commented 1 year ago

I guess I must've been unclear but I thought I made it clear that I recommend for you to set the formatting of all files to what you prefer in order to avoid the situation as described above.

The only missing thing is to decide on one formatting style. I'm not very knowledgeable about lua formatters, I'm not sure if the formatting you have here is the result of your IDE or of some other program.

I just used https://goonlinetools.com/lua-beautifier/, but https://github.com/marketplace/actions/stylua seems to be a good option if you want it automated (which is naturally preferrable).

vendethiel commented 1 year ago

I’ll rerun a format locally. Thanks for the contribution.

vendethiel commented 1 year ago

Hey, did you actually run this on retail at all or not? Seems like it broke cooldowns (at least) because it doesn't know which class/spec people are (even those GLADIUS_SPEC_UPDATE seems to fire just fine). I have some time again so I need to take a look if you haven't tried it.

ManneN1 commented 1 year ago

I did not try this on retail as I made very clear in the PR here:

"Note: I have not tested this on BCC or retail. It might not work there, and the PR might therefore need some more tuning to work correct on the other versions of the game."

vendethiel commented 1 year ago

You're right, forgot about this when I launched retail again. Pushed retail fixes.