DarkflameUniverse / DarkflameServer

The main repository for the Darkflame Universe Server Emulator project.
https://www.darkflameuniverse.org/
GNU Affero General Public License v3.0
639 stars 172 forks source link

ENH: Replace numeric indexing of SQLite columns with column names #1531

Closed EmosewaMC closed 4 months ago

EmosewaMC commented 5 months ago

Is your feature request related to a problem?

Currently, column indexing is prone to issues with off by one, copy paste and ambiguity. This has caused issues in the past and has caused issues to this day and will cause issues in the future if a column is added to the query or removed in an update.

Describe the solution you'd like

Replace any calls of, for example a query with a column banana and a getter getIntField(0) with the getter getIntField("banana"). there are other similar api calls which should have this done as well.

Repository breaking implications

Binary size will grow but we're talking a few hundred bytes. Worth the zero error and runtime error if a column doesnt exist in an updated query.

Describe alternatives you've considered

Why are these not just using the ORM in these random queries? this should be done in the future. No file should need to link with sqlite.

Additional context

i ate a carrot today. tasted good.

jadebenn commented 5 months ago

It may be possible to get both benefits by using some sort of compile-time tagging. I.e. using an enum or constexpr value to index by. So instead of getIntField("banana") perhaps it could be something like getIntField<FooColumn::Banana>(). I'll play around with it when I have time.

aronwk-aaron commented 5 months ago

I'm in favor of just strings, this should really be a set it and forget it long term

jadebenn commented 5 months ago

I think I might have misunderstood the nature of the issue

EmosewaMC commented 5 months ago

I think I might have misunderstood the nature of the issue

The solution does not need to be complex. Just replacing a number with a string instead.