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

Cleanup utils #162

Merged
merged 2 commits into from Aug 27, 2022
Merged

Cleanup utils #162

merged 2 commits into from Aug 27, 2022

Conversation

Serial-ATA
Copy link
Contributor

@Serial-ATA Serial-ATA commented Aug 26, 2022

  • Breaks up vane.util.Util into multiple classes
  • Prevent potential NPEs in resolve_skin/resolve_uuid

Is there a preferred naming style for classes in util? Should they all have a Util suffix?

@oddlama
Copy link
Owner

oddlama commented Aug 26, 2022

Thanks for your PR!
There isn't really a preferred style. When I started this project I didn't anticipate to have that many utils, so they just grew into the state they are at now :P

I'd also happily accept this PR, but I am wondering how you plan to combine the waterfall utils with the vane utils? As the waterfall plugin is started on the proxy server, it has no access to classes from the vane-core package (which is why they were duplicated, because I was lazy). Are you going to shadow them into the waterfall plugin?

@Serial-ATA
Copy link
Contributor Author

Ah, sorry it's not often I work with Java 😅. Is shadowing the best thing to do here?

@oddlama
Copy link
Owner

oddlama commented Aug 26, 2022

I feel you, the ecosystem isn't great.

Shadowing definitely is an option, but don't ask me how to shadow a single file from another project :P
Symbolic linking might be the easiest way to achieve deduplication.

I'm sure there are a plethora of ways to do it directly in the build.gradle, but I won't really be able to help you there. I'm not overly proficient in writing such specific gradle things.

@Serial-ATA
Copy link
Contributor Author

Serial-ATA commented Aug 26, 2022

I just added the utils back to waterfall for now. I gave it a shot, but gradle is too crazy of a beast for me 😄.

Finally figured it out.

@Serial-ATA
Copy link
Contributor Author

Hey, I'm not too familiar with clang-format. I just ran it and the diff is pretty large.

The command I'm using is: clang-format -i vane-waterfall/src/main/java/org/oddlama/vane/waterfall/*.java

Am I doing something wrong or is the formatter just not used often?

@oddlama
Copy link
Owner

oddlama commented Aug 26, 2022

No you are not doing it wrong. We were previously using spotless to format the code, but using it just for formatting was discontinued upstream, which meant we had to abandon it after the switch to Java 17. Prior to spotless I used clang-format.

When I'm programming nowadays I just use the autoformatter of my language server (eclipse.jdt.ls with neovim), so you can just ignore it. Next time is open those files they will get formatted. :)

@oddlama oddlama merged commit 9653c70 into oddlama:develop Aug 27, 2022
@oddlama
Copy link
Owner

oddlama commented Aug 27, 2022

Thanks!

@Serial-ATA Serial-ATA deleted the cleanup-utils branch August 27, 2022 15:49
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.

None yet

2 participants