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

Clarify Rule: Use caseless enums for organizing public or internal constants and functions into namespaces. #218

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

teddy5518
Copy link
Contributor

@teddy5518 teddy5518 commented Mar 30, 2023

Summary

I adjusted two rules's wording, Use caseless `enum`s for organizing `public` or `internal` constants and functions into namespaces and Bind to `self` when upgrading from a weak reference, to reduce confusion.

Reasoning

Bind to `self` when upgrading from a weak reference's example of wrongdoing contains do work key word which confuses if the example is right or wrong. Use caseless `enum`s for organizing `public` or `internal` constants and functions into namespaces was not stating if the case was wrong or right, so I thought adding RIGHT keyword increase readability of the example.

Please react with 👍/👎 if you agree or disagree with this proposal.

README.md Outdated
@@ -401,7 +401,7 @@ _You can enable the following settings in Xcode by running [this script](resourc
func request(completion: () -> Void) {
API.request() { [weak self] response in
guard let strongSelf = self else { return }
// Do work
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why we would remove this, considering:

  1. the example below includes it as well
  2. this is a placeholder that implies the closure contains more code than just completion(). If the closure truly just called completion() immediately then unwrapping self would be unnecessary, making the example redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading your comment, I could understand the true usage of '// Do work'. I will revert change accordingly. Thank you for explaining on this.

@@ -1779,6 +1779,7 @@ _You can enable the following settings in Xcode by running [this script](resourc
Caseless `enum`s work well as namespaces because they cannot be instantiated, which matches their intent.

```swift
// RIGHT
Copy link
Member

@calda calda Mar 30, 2023

Choose a reason for hiding this comment

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

I'm not sure we need this here -- I think it's clear from the context since there's only a single example for this rule. If we added an incorrect example (e.g. using struct instead of enum) I could see why we would want to include the // RIGHT and // WRONG comments.

Copy link
Contributor Author

@teddy5518 teddy5518 Mar 30, 2023

Choose a reason for hiding this comment

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

As my purpose of PR was to increase readability, I agree that providing struct usage of incorrect example could be better. Then may I add below as incorrect examples?

struct Environment {
  static let earthGravity = 9.8
  static let moonGravity = 1.6
}
struct Environment {

  struct Earth {
    static let gravity = 9.8
  }

  struct Moon {
    static let gravity = 1.6
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that example would be incorrect based on this rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for sharing thoughts. I made adjustment accordingly.

@teddy5518 teddy5518 changed the title Clarify Rule: adding Right keyword and deleting confusing word Clarify Rule: Use caseless enums for organizing public or internal constants and functions into namespaces. Mar 30, 2023
@teddy5518 teddy5518 changed the title Clarify Rule: Use caseless enums for organizing public or internal constants and functions into namespaces. Clarify Rule: Use caseless enums for organizing public or internal constants and functions into namespaces. Mar 30, 2023
@calda calda merged commit 79e4488 into airbnb:master Mar 30, 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
2 participants