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

Exposed KVCollectionValue #56

Closed
wants to merge 1 commit into from

Conversation

kingsznhone
Copy link

When doing some data extraction, the operation like

List<KVObject> collection = ((KVCollectionValue)app.Data["config"]["launch"]).children;

was not allowed due to the Internal Access Modifiers.

In some situation, KVCollectionValue is much handy than abstract KVValue

@kingsznhone kingsznhone changed the title Update KVCollectionValue.cs Exposed KVCollectionValue Jul 27, 2022
@yaakov-h
Copy link
Member

yaakov-h commented Jul 28, 2022

Publicly exposing the inner List ties us to that particular implementation forever, and allows consumers to bypass any validation on the members (currently that is just nullability).

What is your actual use-case?

@kingsznhone
Copy link
Author

kingsznhone commented Aug 1, 2022

I'm analyzing appinfo.vdf. With SteamAppInfoParser
There are many games with multiple start options.
When I'm trying to get index of each start option.
I Must use Indexer like var options = AppData["config"]["launch"] that return a KVValue (actually KVCollectionValue)
This KVValue Contains multiple children with diffirient name (int index number).

The index number was not always increment by 1.
I don't know how many items insides options.
So I can't keep using indexer like options["index"]

The only way is to iterate the options.children to exract infomations that I'm intrerested in.
I can't do that without exposed iterable properties.

This situation is similar when analyzing libraryfolders.vdf

@yaakov-h
Copy link
Member

yaakov-h commented Aug 1, 2022

Casting it to IEnumerable<KVObject> should let you enumerate the children, giving you access to both the names and values.

@kingsznhone
Copy link
Author

kingsznhone commented Aug 1, 2022

Casting it to IEnumerable<KVObject> should let you enumerate the children, giving you access to both the names and values.

Thanks. That helps.
But I'll keep my opion on Publicly exposing.
This kind of explicit conversion seems too obscured than List<T> to me.
this might be a programming philosophy that I can't understand.

What if adding ToList() method on Class KVValue with inner Type judgment.

If there is a collection value, return a list, otherwise throw exception.

@xPaw
Copy link
Member

xPaw commented Aug 1, 2022

See #21

@kingsznhone
Copy link
Author

kingsznhone commented Aug 1, 2022

See #21

yea, casting IEnumerable<T> makes APIs more complicated.

There is another confused method public override int ToInt32(IFormatProvider provider)

Although I can call value.ToInt32(null).
But I think adding a value.ToInt32() method is much more convenient and clear.
Handy to consumers, without digging into source code.

@yaakov-h
Copy link
Member

yaakov-h commented Aug 1, 2022

You should be able to simply cast the value to int if you don't have a particular culture to use etc.:

https://github.com/SteamDatabase/ValveKeyValue/blob/master/ValveKeyValue/ValveKeyValue.Test/KVObjectCastTestCase.cs#L103

@yaakov-h yaakov-h mentioned this pull request Jun 8, 2023
@yaakov-h
Copy link
Member

yaakov-h commented Jun 8, 2023

Closing in favour of #30 to provide a more wholistic API redesign.

@yaakov-h yaakov-h closed this Jun 8, 2023
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.

3 participants