BrassAmber-Mods / BrassArmory

3 stars 1 forks source link

Fixed Laser Arrow #5

Closed JMilamber closed 2 years ago

JMilamber commented 2 years ago
  • I'm not sure I entirely agree with all of the formatting and refactoring changes being mixed in with the logic changes. I'd say if we can from here on out let's try and avoid refactors like this (myself included) without consulting the others and also putting it into a separate PR, otherwise we can't work parallel and ensuring branches stay up to date (like the quiver branch) would be virtually impossible since packages keep moving.

My apologies, I gave Der Toaster the go-ahead to re-arrange stuff without thinking about it, still adjusting to having so many people working on a project at once.

These changes were also not meant to be added to this Pull Request, (this was supposed to be just for the laser arrow) but as the laser arrow was reworked in the melee branch all of Der Toasters' work got added to the pull request.

Unsure if I should remove this pull request, or if we should try and commit this before further work is done, so that everyone can work off of the same folder structure. @DerToaster98 @FireController1847 @Xratedjunior

FireController1847 commented 2 years ago

While I was exploring the source the refactor seemed to be pretty good so I'd be okay with just including it in this PR. I don't think merging this PR will put any major stops or hurdles for other things regarding this project, so no worries. Honestly I probably wouldn't have even thought about it either - I didn't when I made PR 3 haha. You live you learn, right?

DerToaster98 commented 2 years ago

What exactly do you mean by "formatter"? Do you just mean eclipse's built-in tool or a third party software? Cause for the first option i can provide the settings we use in cqr, that way all parties use the same formatting and it won't cause more issues.

FireController1847 commented 2 years ago

What exactly do you mean by "formatter"? Do you just mean eclipse's built-in tool or a third party software? Cause for the first option i can provide the settings we use in cqr, that way all parties use the same formatting and it won't cause more issues.

I'm using IntelliJ. Most notably I just right click on the package and hit "reformat code." Also I use the auto import optimizer. Not sure how to go about ensuring formatting stays the same between both programs

DerToaster98 commented 2 years ago

It would be really appreciated if you could just adjust it so that it will just format code (e.g. editing line breaks and indentation(?)), i use auto import too but i'm not a fan of the star imports mentioned earlier (imo really just creates overhead when not everything from a package is used, also it decreases understandability and readability of the whole code)

So i think it should just work if the options for modifying the declarations are toggled off

FireController1847 commented 2 years ago

It would be really appreciated if you could just adjust it so that it will just format code (e.g. editing line breaks and indentation(?)), i use auto import too but i'm not a fan of the star imports mentioned earlier (imo really just creates overhead when not everything from a package is used, also it decreases understandability and readability of the whole code)

So i think it should just work if the options for modifying the declarations are toggled off

I'll check and see if I can do that. I've never had an issue with star imports due to compiler optimization, it only uses the needed packages the star import is simply to clean up the list of imports at the top of the list (realistically anymore, I understand that's not what it actually means but due to compiler optimization it doesn't require any more packages than necessary anyways, it just shortens the reference). Though I'm not sure I understand the argument against understandability and readability. How often are you actually looking at the imports for.. readability?

For now, I'll only format the files I work with personally and try and keep it separate from yours, maybe we can try and do the same for each to get the best of both worlds?

DerToaster98 commented 2 years ago

On star imports: Yes, the compiler optimizes that. I look at the package way too often though. And normally i import packages using auto-import anyway

At the end of the day it is preference if one uses them or not, imo they just fall in the category of "syntax suggar" anyway.

I approve your suggestion, but we need something that keeps track of "who is working on what" (or i'm totally blind and don't see it)

FireController1847 commented 2 years ago

@DerToaster98 Maybe we could utilize Javadocs @author annotation for classes? Then we can list authors for certain files. That could also be helpful if a specific file needs to be fixed or changed so the "owner" of that file can be consulted / notified.

DerToaster98 commented 2 years ago

Sounds good, we should define a class header then, used that in cqr when we were more than 2 developers and it helped indeed with identifying who is responsible. I'll search it after work and send it here, we might need to change it a bit though.

JMilamber commented 2 years ago

I like this idea for author tracking a lot 👍. I have also created a new issue which contains who is working on which branches and a couple of rules, (basically what we've outlined here) so we are all on the same page and if anyone else joins they can be redirected there.

If something needs to be worded better or changed or a rule added let me know on the issue.