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

[API Proposal]: Add Length and IsEmpty properties to System.BinaryData #77970

Closed
0xced opened this issue Nov 7, 2022 · 6 comments · Fixed by #86721
Closed

[API Proposal]: Add Length and IsEmpty properties to System.BinaryData #77970

0xced opened this issue Nov 7, 2022 · 6 comments · Fixed by #86721
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@0xced
Copy link
Contributor

0xced commented Nov 7, 2022

Background and motivation

Getting the length of a System.BinaryData instance is something quite natural. Currently, the best way to get the length is by doing data.ToMemory().Length which is not very pleasant.

There's also a trap where one could do data.ToArray().Length or even data.ToStream().Length and this would allocate!

Having a Length property solves both issues: code is more readable and ensures no allocation.

An IsEmpty property would also be welcome.

API Proposal

namespace System;

public class BinaryData
{
    public int Length { get; }
    public bool IsEmpty { get; }
}

API Usage

void ProcessData(BinaryData data, string outputFile)
{
    if (data.IsEmpty)
    {
        Console.WriteLine($"No data available to write to {outputFile}");
    }
    else
    {
        Console.WriteLine($"Writing {data.Length} bytes to {outputFile}");
        // ... actual implementation ...
    }
}

Alternative Designs

No response

Risks

There are no risks associated with this proposition. The implementations of those properties are trivial and would just call respectively _bytes.Length and _bytes.IsEmpty.

@0xced 0xced added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 7, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 7, 2022
@ghost
Copy link

ghost commented Nov 7, 2022

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Getting the length of a System.BinaryData instance is something quite natural. Currently, the best way to get the length is by doing data.ToMemory().Length which is not very pleasant.

There's also a trap where one could do data.ToArray().Length or even data.ToStream().Length and this would allocate!

Having a Length property solves both issues: code is more readable and ensures no allocation.

An IsEmpty property would also be welcome.

API Proposal

namespace System;

public class BinaryData
{
    public int Length { get; }
    public bool IsEmpty { get; }
}

API Usage

void ProcessData(BinaryData data, string outputFile)
{
    if (data.IsEmpty)
    {
        Console.WriteLine($"No data available to write to {outputFile}");
    }
    else
    {
        Console.WriteLine($"Writing {data.Length} bytes to {outputFile}");
        // ... actual implementation ...
    }
}

Alternative Designs

No response

Risks

There are no risks associated with this proposition. The implementations of those properties are trivial and would just call respectively _bytes.Length and _bytes.IsEmpty.

Author: 0xced
Assignees: -
Labels:

api-suggestion, area-System.Memory, untriaged

Milestone: -

@0xced
Copy link
Contributor Author

0xced commented Apr 19, 2023

Is there anything I should add to the proposal to initiate a discussion? Some months later I have worked with BinaryData again and struggled with this exact same issue again.

Ping @jeffhandley

@adamsitnik adamsitnik added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels May 18, 2023
@adamsitnik adamsitnik self-assigned this May 18, 2023
@adamsitnik
Copy link
Member

Is there anything I should add to the proposal to initiate a discussion? S

Apologies for the delay. The proposal looks good to me, I've marked it as ready for review. Looking at https://apireview.net/backlog I estimate that it should get reviewed within next two weeks.

@0xced thank you!

@bartonjs
Copy link
Member

bartonjs commented May 18, 2023

Video

Looks good as proposed.

We briefly discussed Length vs Count, and feel that Length is more appropriate here because it's representing a fixed size window of data, like Span/Memory/Array, vs a point-in-time number like List and the other collections.

namespace System;

public class BinaryData
{
    public int Length { get; }
    public bool IsEmpty { get; }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 18, 2023
@adamsitnik adamsitnik removed their assignment May 19, 2023
@adamsitnik adamsitnik added good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels May 19, 2023
0xced added a commit to 0xced/runtime that referenced this issue May 24, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 24, 2023
@0xced
Copy link
Contributor Author

0xced commented May 24, 2023

A pull request addressing this issue is ready: #86721

@adamsitnik adamsitnik added this to the 8.0.0 milestone May 25, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 25, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants