surrealdb / surrealdb.java

SurrealDB SDK for Java
https://surrealdb.com
Apache License 2.0
67 stars 21 forks source link

Use modern Java and cleanups #78

Closed JFortun closed 7 months ago

JFortun commented 1 year ago

This PR is a refactor of the project to:

phughk commented 1 year ago

Hey thanks! Will merge this once conflicts are resolved! 🎉

JFortun commented 1 year ago

Hey thanks! Will merge this once conflicts are resolved! 🎉

Hello @phughk, conflicts resolved.

JFortun commented 1 year ago

@phughk Can you assign this PR to me? I don't have access.

phughk commented 1 year ago

Actually just gave it a proper read (yesterday was at the hacktoberfest event so split attention). I thought this was just spotless apply - not sure why variables inside blocks are being made final? They are "final" in a way, but constantly writing final is terse and we don't need the keyword because it isn't exposed beyond the function. So it is unnecessary and having final made explicit may give the wrong impression that its deliberate (when I believe what you mean is that it isn't modified i.e. implicit). Could you revert the final keywords please? RE assigning - assigning is for who owns the review. Will assign to me.

JFortun commented 1 year ago

Actually just gave it a proper read (yesterday was at the hacktoberfest event so split attention). I thought this was just spotless apply - not sure why variables inside blocks are being made final? They are "final" in a way, but constantly writing final is terse and we don't need the keyword because it isn't exposed beyond the function. So it is unnecessary and having final made explicit may give the wrong impression that its deliberate (when I believe what you mean is that it isn't modified i.e. implicit).

Could you revert the final keywords please?

RE assigning - assigning is for who owns the review. Will assign to me.

Thanks for the review @phughk.

The final keywords serve a purpose, even if the variable is not exposed beyond the function, the JVM is able to do some optimizations when we set a varible as final. Here you have some benchmarks and info about the advantages

About assigning, that's not for the one that does the review. Who owns the review goes in the "reviewers" field just above. The PRs are assigned to the ones working on them. Each field has their purpose.

phughk commented 1 year ago

Based on the 2 answers in SO (not docs), people say that assignees are the "stewards" or go to owners of the PR lifecycle (rejected, timely reviews, merging etc). https://stackoverflow.com/questions/41087206/on-github-whats-the-difference-between-reviewer-and-assignee That's how I've been using it so far. The reviewers are just a flexible list of people who need to or have reviewed.

Thanks for sharing about the final stuff I had no idea. Makes a lot of sense :) Re-reviewing now