Skip to content

FileSystemStream Issues #943

Description

@JasonBock

Describe the bug
There's a documentation and API issue with this type. There isn't a way to reproduce this, more that the design should be reviewed.

  • The XML docs say Wraps the <see cref="FileStream" />, but the wrapped field, _stream, is of type Stream. There's no runtime check to ensure that the stream provided in the constructor is of type FileStream. This is confusing because the docs state one thing, but the implementation doesn't seem to match that.
  • The path parameter for the constructor is defined as string?, but the constructor will throw an ArgumentNullException if path is null. This is confusing, as annotating a parameter as nullable means that the method can accept a null value. If path cannot be null, then remove the nullable annotation.

Activity

  1. vbreuss commented on Feb 9, 2023

    @vbreuss
    Member

    Thanks for the hint @JasonBock.

    The intention of this class is to wrap the FileStream but make it testable. So the interface wraps the FileStream and in the "normal" usage in production the provided stream should be a FileStream.
    Only in the testing wrapper, we will replace it with a different stream for testing purposes.
    I agree, that the XML documentation is short and could be improved, but the intention of it wrapping a FileStream is in essence correct.

    With your second point I agree completely. The path parameter should be string.

    A pull request would be welcome!

  2. added
    type: enhancementIssues that propose new functionality
    state: ready to pickIssues that are ready for being worked on
    area: coreIssues that address the core abstractions & the wrappers
    flag: good-first-issueIssues that are good for first time contributors
    and removed
    type: bugIssues that describe misbehaving functionality
    on Feb 14, 2023
  3. ZaffyPuck commented on Apr 7, 2023

    @ZaffyPuck

    Hi, I am a college student looking trying to get more involved with open source and looking for a first-time "bug fix" and figured I could help out with this! I am a little confused with what XML docs you are referring to. Perhaps a link would help. As for the string issue, it should be string as opposed to string? because you want the user to pass data through the constructor. Is there anything I could help with as far as fixing the documentation or code? Let me know!

  4. vbreuss commented on Apr 10, 2023

    @vbreuss
    Member

    @ZaffyPuck : The second point was already fixed in #956!
    A link to the file is mentioned in the description.

  5. added a commit that references this issue on Apr 18, 2023
    8ad2e98
  6. github-actions commented on Apr 19, 2023

    @github-actions

    This is addressed in release v19.2.15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: coreIssues that address the core abstractions & the wrappersflag: good-first-issueIssues that are good for first time contributorsstate: ready to pickIssues that are ready for being worked onstate: releasedIssues that are releasedtype: enhancementIssues that propose new functionality

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions