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.

Sign up for GitHub

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

Jump to bottom

feat: Add String(contentsOf: FilePath ...) #3048

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Add String(contentsOf: FilePath ...) #3048

wants to merge 6 commits into from

Conversation

natikgadzhi
Copy link
Contributor

Motivation:

Makes it possible to read contents of an NIOFileSystem.FilePath into a String directly. Closes #3010.

Modifications:

  • Added a new String+FileSystem.swift into NIOFileSystem target.
  • Added basic tests to the existing ConvenienceTests.swift.
  • Added a test to cover Array+FileSystem initializer as well.

Result:

  • String initializer for FilePath is now available
  • Array initializer for FilePath now has a test

/cc @weissi

@natikgadzhi
feat: Add String(contentsOf: FilePath ...)
f671903 
Motivation:

Makes it possible to read contents of an NIOFileSystem.FilePath into a String directly.

Modifications:

- Added a new `String+FileSystem.swift` into `NIOFileSystem` target.
- Added basic tests to the existing `ConvenienceTests.swift`.
- Added a test to cover `Array+FileSystem` initializer as well.

Result:

- String initializer for `FilePath` is now available
- Array initializer for `FilePath` now has a test
@weissi weissi requested a review from glbrntt January 13, 2025 09:37
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jan 13, 2025
Lukasa
Lukasa reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm 100% fine with this change, but given that we're considering using a type other than FilePath in this module, I wonder if we should sit on this a little bit. @glbrntt?

@weissi
Copy link
Member

weissi commented Jan 13, 2025

So I'm 100% fine with this change, but given that we're considering using a type other than FilePath in this module, I wonder if we should sit on this a little bit. @glbrntt?

@Lukasa This is in the _NIOFileSystem module which

  • currently uses FilePath
  • is not API-stable (_...)
  • will probably get a big change (s/FilePath/NIOFilePath/) at some point soon

So I would argue that it's better to merge this as it lessens the likelihood that this gets forgotten. And because it's in the underscored module, there's no risk to paint ourselves into a corner.

@Lukasa
Copy link
Contributor

Lukasa commented Jan 13, 2025

It's the big change that I'm concerned about: I wonder whether it's particularly egregious to add a method whose immediate future is to be deprecated or removed in favour of one with a different type.

@glbrntt
Copy link
Contributor

glbrntt commented Jan 13, 2025

It's the big change that I'm concerned about: I wonder whether it's particularly egregious to add a method whose immediate future is to be deprecated or removed in favour of one with a different type.

I'd be more concerned if this was isolated to a single API, but given we'll have to deprecate and replace many APIs I don't really see this as an issue.

@weissi
Copy link
Member

weissi commented Jan 13, 2025

It's the big change that I'm concerned about: I wonder whether it's particularly egregious to add a method whose immediate future is to be deprecated or removed in favour of one with a different type.

I'd be more concerned if this was isolated to a single API, but given we'll have to deprecate and replace many APIs I don't really see this as an issue.

+1, same.

glbrntt
glbrntt requested changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for opening this! DocC is unhappy about a reference and Johannes' comment re: the copyright header needs addressing but otherwise this looks great.

Sources/NIOFileSystem/String+FileSystem.swift Outdated
/// - maximumSizeAllowed: The maximum size of file which can be read, in bytes, as a ``ByteCount``. If the file is larger than this, an error is thrown.
/// - fileSystem: The ``FileSystemProtocol`` instance to use to read the file.
///
/// - Throws: If the file is larger than `maximumSizeAllowed`, an ``FileSystemError.resourceExhausted`` error will be thrown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocC is unhappy with the FileSystemError.resourceExhausted here. I think you need to say that an error with code FileSystemError/Code-swift.struct/resourceExhausted is thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL. Seems to work, pushing up now.

@natikgadzhi natikgadzhi requested a review from glbrntt January 14, 2025 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lukasa Lukasa Lukasa left review comments

@weissi weissi weissi left review comments

@glbrntt glbrntt Awaiting requested review from glbrntt

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NIOFileSystem: missing String(contentsOf:maximumSizeAllowed:)
4 participants
@natikgadzhi @weissi @Lukasa @glbrntt