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

Update protobufs #827

Merged
merged 5 commits into from
May 9, 2020
Merged

Update protobufs #827

merged 5 commits into from
May 9, 2020

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Apr 3, 2020

  1. Skipped Underlords protos because it requires some generation changes.
  2. Removed EnumeratePublishedFiles because all the protos for it are gone; can't obsolete it because CMsgCREEnumeratePublishedFilesResponse was removed and it won't compile.
  3. Combined SteamKit2.Unified.Internal to SteamKit2.Internal because some protos depend on each other and they're in separate namespaces.
    If this change is not acceptable, then protobuf generation needs to be upgraded to allow adding usings.

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #827 into master will increase coverage by 0.13%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
+ Coverage   22.75%   22.89%   +0.13%     
==========================================
  Files          95       95              
  Lines        9344     9283      -61     
  Branches      780      779       -1     
==========================================
- Hits         2126     2125       -1     
+ Misses       7084     7024      -60     
  Partials      134      134              
Impacted Files Coverage Δ
...SteamKit2/Steam/Handlers/SteamFriends/Callbacks.cs 0.00% <ø> (ø)
...dlers/SteamUnifiedMessages/SteamUnifiedMessages.cs 7.76% <0.00%> (ø)
...t2/SteamKit2/Steam/Handlers/SteamUser/Callbacks.cs 2.77% <ø> (ø)
...teamKit2/Steam/Handlers/SteamWorkshop/Callbacks.cs 0.00% <ø> (ø)
...Kit2/Steam/Handlers/SteamWorkshop/SteamWorkshop.cs 12.32% <ø> (+2.98%) ⬆️

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 9e9c82e...ab629fa. Read the comment docs.

@yaakov-h
Copy link
Member

yaakov-h commented Apr 18, 2020

I think this makes sense, but wouldn't mind a second opinion.

What changes need to be made for Underlords?

@xPaw
Copy link
Member Author

xPaw commented Apr 18, 2020

What changes need to be made for Underlords?

Needs to bring in a bunch of more proto files like user messages, can't be bothered to deal with it.

@voided
Copy link
Member

voided commented Apr 18, 2020

I think this makes sense, but wouldn't mind a second opinion.

Most of this looks good to me as well - I think I originally suggested merging the internal namespaces on IRC. They're internal anywhere so the implication is that breakage here from time to time is expected.

I could go either way on EnumeratePublishedFiles - should it be an Obsolete stub that does a no-op, should it throw an exception, or should it be removed entirely? All three approaches seem fine.

@yaakov-h
Copy link
Member

An obsolete stub could just cause consumers to leak resources, waiting for an event that never fires.

Throwing an exception sounds good, but removing the method will also cause an exception to be thrown... just far more cryptically.

I think all-up I'd rather keep the method, make it throw an exception, mark as obsolete and optionally hide it with EditorBrowsableAttribute.

@xPaw
Copy link
Member Author

xPaw commented May 8, 2020

Anything blocking this? There are proto updates that I'd like to use.

@yaakov-h yaakov-h merged commit fb3c65a into SteamRE:master May 9, 2020
@xPaw xPaw deleted the protobufs2 branch May 9, 2020 09:13
@yaakov-h yaakov-h added this to the 2.3.0 milestone May 16, 2020
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.

4 participants