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

Reverse Data -> String conversion rule #5601

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

Conversation

samrayner
Copy link
Contributor

This reverses the Data -> String portion of this PR by introducing a new rule, following feedback on the original issue. It leaves the String -> Data portion of the original rule unchanged.

@samrayner samrayner force-pushed the samrayner/5263-reverse-data-string-conversion-rule branch from 8f22541 to 6bca4f2 Compare May 23, 2024 06:33
@samrayner samrayner force-pushed the samrayner/5263-reverse-data-string-conversion-rule branch from 6bca4f2 to f0d657d Compare May 23, 2024 06:48
…ersionRule.swift

Co-authored-by: Ben P. <benj.c.palmer@gmail.com>
Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

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

Other than some nits on formulation and style, this looks good and reasonable to me. Thank you for taking immediate (re)action on the comments in the original PR!

@@ -10,7 +10,10 @@

#### Enhancements

* None.
* Add new `optional_data_string_conversion` rule to enforce
failable conversions of `Data` -> UTF-8 `String`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please append two spaces to the end of the description. They are required to enforce a line break in the rendered Markdown.

Comment on lines +30 to +31
* Revert `optional_data_string_conversion` enforcing
non-failable conversions of `Data` -> UTF-8 `String`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Revert `optional_data_string_conversion` enforcing
non-failable conversions of `Data` -> UTF-8 `String`.
* Revert the part of the `non_optional_string_data_conversion`
rule that enforces non-failable conversions of `Data` -> UTF-8
`String`. This is due to the fact that the data to be converted
can be arbitrary and especially doesn't need to represent a valid
UTF-8-encoded string.

Please append two spaces to the end of the description. They are required to enforce a line break in the rendered Markdown.

Also, this should go into the "Breaking" section.

@@ -5,16 +5,14 @@ struct NonOptionalStringDataConversionRule: Rule {
var configuration = SeverityConfiguration<Self>(.warning)
static let description = RuleDescription(
identifier: "non_optional_string_data_conversion",
name: "Non-Optional String <-> Data Conversion",
description: "Prefer using UTF-8 encoded strings when converting between `String` and `Data`",
name: "Non-Optional String -> Data Conversion",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: "Non-Optional String -> Data Conversion",
name: "Non-optional String -> Data Conversion",

name: "Non-Optional String <-> Data Conversion",
description: "Prefer using UTF-8 encoded strings when converting between `String` and `Data`",
name: "Non-Optional String -> Data Conversion",
description: "Prefer using non-optional Data(_:) when converting a UTF-8 String to Data",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "Prefer using non-optional Data(_:) when converting a UTF-8 String to Data",
description: "Prefer non-optional `Data(_:)` initializer when converting `String` to `Data`",

static let description = RuleDescription(
identifier: "optional_data_string_conversion",
name: "Optional Data -> String Conversion",
description: "Prefer using failable String(data:encoding:) when converting from `Data` to a UTF-8 `String`",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "Prefer using failable String(data:encoding:) when converting from `Data` to a UTF-8 `String`",
description: "Prefer failable `String(data:encoding:)` initializer when converting `Data` to `String`",

@SimplyDanny
Copy link
Collaborator

Please run IntegrationTests locally and fix any violations/failures reported.

@SimplyDanny SimplyDanny linked an issue Jun 10, 2024 that may be closed by this pull request
2 tasks
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.

Rule Request: Prefer non-optional UTF8 String <-> Data conversion
3 participants