search

the-marenga / sf-server

A Shakes & Fidget server in the very early stages of developement
3 stars 1 forks source link

Add logging & Fix some bugs #8

Closed GutZuFusss closed 5 days ago

GutZuFusss commented 6 days ago

Sorry for having this in one PR, but I've at least split it into seperate commits, so of course feel free to cherry pick.

For the fixes, I am not sure if I am just really really dumb, but I really don't see any way how this could ever have worked with the official client before. The "Equipment" FK violating the "NOT NULL" constraint in particular I don't see how it was ever under any circumstances working, except maybe you used a different DB schema than the one checked into this repo.

On the other hand, I think you will be able to quickly verify the general functionality (and the old version being broken :D) with the steps I outlined in this file from my previous PR.

In short, I think this solution is the most natural. It leaves the most possible ways to continue with regards to sourcing the clients without being overly complicated. The concept is simple:

Here is what that looks like on my german Edge: image

GutZuFusss commented 6 days ago

Ok, looking at the conflicts it honestly faster if I just do a (or multiple if you think the logging is good too) new PR with everything from this PR, should only take me about 10 minutes I think. The brain work is already done, just some copy here, some paste there 👯

Most important for me tho is to know if this was neccessary or just good for readability or just stupid: https://github.com/the-marenga/sf-server/pull/8/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR334

GutZuFusss commented 5 days ago

Closing this, all the bugs are now fixed, and the logging we will discuss in the corresponding issue :)

For me personally the only thing I am still curious about is what someone (@the-marenga :D) who knows rust has to say about this: https://github.com/the-marenga/sf-server/pull/8/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR334

the-marenga commented 5 days ago

I should probably have mentioned, that the development state I was refering to was on a secondary branch. I was not expecting such a development effort immediately. I am actually sorry about that.

The main thing is that I switched to sqlite and axum over the last week. That makes development a lot easier going forward, since actix (as you have noticed) has a few issues around parsing and overall compatibility. Same thing with sqlx, which is nice in a lot of cases, but leads to long compile times, hard compiler errors and requires a working postgres db + credentials setup, which is very annoying for something, that realistically is never going to be in production.

The effort to improve logging looks good, but othewise I am just not going to be able to merge this as is.

Most important for me tho is to know if this was neccessary or just good for readability or just stupid: https://github.com/the-marenga/sf-server/pull/8/files#diff-42cb6807ad74b3e201c5a7ca98b911c5fa08380e942be6e4ac5807f8377f87fcR334

The fixes around inserting quests? They look good. Overall all the error handling and better matching around the sqlx requests make perfect sense. The fact, that this was required, was a big reason why I switched away from that though. Having to manually match & error handle every db request is just too annoying long term. Otherwise all of this would make perfect sense.