-
Notifications
You must be signed in to change notification settings - Fork 496
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
Remove only_public from pics requests #911
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level it looks OK but this is both a bincompat and srccompat breaking change. We'll need to carefully call this out in release notes.
As for making it a struct - seems like a decent idea, I don't see any downsides.
/// <param name="metaDataOnly">Whether to send only meta data.</param> | ||
/// <returns>The Job ID of the request. This can be used to find the appropriate <see cref="PICSProductInfoCallback"/>.</returns> | ||
public AsyncJobMultiple<PICSProductInfoCallback> PICSGetProductInfo(uint? app, uint? package, bool onlyPublic = true, bool metaDataOnly = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a source-breaking change for any calls like PICSGetProductInfo(1, 2, true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never liked these bools, as overload that takes IEnumerable<PICSRequest
did not have onlyPublic
already.
/// <param name="metaDataOnly">Whether to send only meta data.</param> | ||
/// <returns>The Job ID of the request. This can be used to find the appropriate <see cref="PICSProductInfoCallback"/>.</returns> | ||
public AsyncJobMultiple<PICSProductInfoCallback> PICSGetProductInfo( IEnumerable<uint> apps, IEnumerable<uint> packages, bool onlyPublic = true, bool metaDataOnly = false ) | ||
public AsyncJobMultiple<PICSProductInfoCallback> PICSGetProductInfo( IEnumerable<uint> apps, IEnumerable<uint> packages, bool metaDataOnly = false ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source-breaking as above.
@@ -263,7 +239,7 @@ public AsyncJobMultiple<PICSProductInfoCallback> PICSGetProductInfo( IEnumerable | |||
var appinfo = new CMsgClientPICSProductInfoRequest.AppInfo(); | |||
appinfo.access_token = app_request.AccessToken; | |||
appinfo.appid = app_request.ID; | |||
appinfo.only_public_obsolete = app_request.Public; | |||
appinfo.only_public_obsolete = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just omit it entirely here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but I don't know what Valve defaults to on the backend, if at all (since it doesn't seem to have an effect).
See #910
It was renamed to
only_public_obsolete
in protos, and I believe it has no effect.Question: Is it a good idea to make
PICSRequest
a struct?