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

Added getting the CellID in the constructor method. #722

Merged
merged 3 commits into from
Oct 2, 2019
Merged

Added getting the CellID in the constructor method. #722

merged 3 commits into from
Oct 2, 2019

Conversation

ADustyOldMuffin
Copy link
Contributor

closes #690

This is my first contribution so I'm still getting used to the project so any constructive criticism is welcome!

All I did was instantiate a configuration in the LogOnDetails and AnonymousLogOnDetails constructors and grab the CellID and set it. Let me know if I should've set the default value elsewhere or if there's a better way to do this.

@yaakov-h yaakov-h self-requested a review October 1, 2019 19:50
@yaakov-h
Copy link
Member

yaakov-h commented Oct 1, 2019

Creating a new configuration and then immediately using it's values is fairly pointless, as it doesn't give the consumer of the library any chance to configure it.

Inside SteamUser, where we use the values from LogOnDetails and AnonymousLogOnDetails, we have access to the configuration object that the consumer has already configured through this.Client.Configuration.

A better approach would be where we use the values at this line:

logon.Body.cell_id = details.CellID;

to either check for the magic value zero and then use the configuration instead, or to make the value on the xxxDetails objects nullable and then perform a null-coalesce like so:

logon.Body.cell_id = details.CellID ?? Client.Configuration.CellID;

This will set a default value if none has been set for CellID inside of the login details.
@codecov-io
Copy link

Codecov Report

Merging #722 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #722      +/-   ##
==========================================
+ Coverage   22.63%   22.69%   +0.05%     
==========================================
  Files          93       93              
  Lines        9568     9572       +4     
  Branches      795      795              
==========================================
+ Hits         2166     2172       +6     
+ Misses       7270     7268       -2     
  Partials      132      132
Impacted Files Coverage Δ
...t2/SteamKit2/Steam/Handlers/SteamUser/SteamUser.cs 27.98% <100%> (+2.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38dd2fd...8af1d36. Read the comment docs.

@ADustyOldMuffin
Copy link
Contributor Author

ADustyOldMuffin commented Oct 1, 2019

I figured it was simpler than what I was doing to get the configuration, embarrassed I missed where to access it. Thanks for the help!

@yaakov-h yaakov-h merged commit 02faf6f into SteamRE:master Oct 2, 2019
@yaakov-h
Copy link
Member

yaakov-h commented Oct 2, 2019

Thanks!

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.

SteamConfiguration.CellID is not used in login
3 participants