Properties and Server Configuration access cleanup

CloudburstMC / Cloudburst

Cloudburst Server is a Bedrock first server software. Originally forked from Cloudburst Nukkit.

178 stars 70 forks source link

Properties and Server Configuration access cleanup #70

Closed johnbanq closed 4 years ago

johnbanq commented 4 years ago

Goal: migrate properties and server configuration access logic out of Server class, to reduce its length and API surface, hence make the dev's life easier(?)

I would also like to propose a more explicit API to simplify property access.

Status: here is the proposed encapsulation scheme to server.properties all usage inside the Server class has been replaced, waiting for opinions on API style and design from the devs before further replacing all usage in the project and do the same to nukkit.yaml

[x] further propagate the refactor to the entire codebase
[x] do the same to nukkit.yaml

Further idea: I think we should provide a facade class, perhaps named ServerConfiguration to move all the getIp() style getter out of the Server class?

johnbanq commented 4 years ago

note: the previous one was closed because I forgot to set it as a draft

johnbanq commented 4 years ago

@lukeeey Here is the codebase that encapsulated the cloudburst.yml and with my facade idea applied.

Is the design of the facade class (ServerConfig.class) acceptable?

How the sub-config classes are created is temporary. I think it is more important to get the API right at first, and we can work on details like this later

Further TODO:

[x] merge the config inside the server.properties into the sub-config classes too
that would provide a more uniform API, but mapping is going to be an issue
[ ] (might need another PR) redesign the cloudburst.yml content
with the config access logic all pulled to one place, I think we can reorg the yaml's content a little bit, to reduce the clauses and make things easier to discover

johnbanq commented 4 years ago

since pulling all that string-based property access all into one place is more important than bikeshedding the detail of the encapsulated API, I think this PR is ready to merge. The TODO items above, if found acceptable by the owner, will be the goal of another PR

johnbanq commented 4 years ago

I think this PR is complete by now, please review so we can get this into mainline ASAP :)

lukeeey commented 4 years ago

Oof, i forgot to click send on my review