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

Load & use DB config in recordsets #226

Merged
merged 6 commits into from
May 23, 2024
Merged

Load & use DB config in recordsets #226

merged 6 commits into from
May 23, 2024

Conversation

twostars
Copy link
Contributor

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the canary branch (left side). Also you should start your branch off our canary.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Description

This behaves similarly to official, with the only difference it uses a shared method in CServerDlg (etc.) to construct the connection string from members of CServerDlg (etc.), whereas officially it accesses this externally, e.g.:

CString strConnect;
strConnect.Format(..., m_pMain->m_strGameDSN, ...);

💔Thank you!

@twostars twostars marked this pull request as ready for review May 22, 2024 07:06
@twostars twostars marked this pull request as draft May 22, 2024 19:39
@twostars twostars marked this pull request as ready for review May 22, 2024 19:39
@twostars twostars marked this pull request as draft May 22, 2024 23:27
@twostars twostars marked this pull request as ready for review May 22, 2024 23:27
twostars added 4 commits May 23, 2024 12:44
This behaves similarly to official, with the only difference it uses a shared method in CServerDlg (etc) to construct
the connection string from members of CServerDlg, whereas officially it accesses this externally, e.g.:

CString strConnect;
strConnect.Format(..., m_pMain->m_strGameDSN, ...);
@twostars twostars marked this pull request as draft May 23, 2024 03:34
@twostars twostars marked this pull request as ready for review May 23, 2024 03:34
Copy link
Member

@stevewgr stevewgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @twostars 🚀 and also thanks for your efforts in explaining how it was done officialy and wrongly xD I like your approach more with the cerntralized function to get the connection string, as well as the singleton approach. That will be also helpful in the long run as more code is added later.
Just one nit-pick comment, let me know if you have the patience to apply, otherwise I'll just merge it.

src/server/AIServer/AIServerDlg.cpp Outdated Show resolved Hide resolved
@twostars twostars marked this pull request as draft May 23, 2024 05:26
@twostars twostars marked this pull request as ready for review May 23, 2024 05:26
@twostars twostars marked this pull request as draft May 23, 2024 05:35
@twostars twostars marked this pull request as ready for review May 23, 2024 05:35
Copy link
Member

@stevewgr stevewgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@stevewgr stevewgr merged commit 59546c5 into ko4life-net:master May 23, 2024
1 check passed
stevewgr added a commit that referenced this pull request Nov 20, 2024
This change updates the LoginServer connection string to be loaded
from server.ini instead of Version.ini. This ensures all server files
share a unified configuration source, improving maintainability.

This adjustment also fixes twostars changes in PR #226.

Future improvements to better structure sections across server files
will follow in a separate commit.
@stevewgr
Copy link
Member

Looks like some stuff were missed in this PR and I fixed it here for the record: #268

in this commit: 1fa93c7

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.

2 participants