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

#5461 added ib_segue_action case in TypeContent #5524

Merged

Conversation

dk-talks
Copy link
Contributor

@dk-talks dk-talks commented Apr 2, 2024

Hey! I have added a new case to TypeContent enum for ib_segue_action. And also adapted the change in TypeContentsOrderRule.swift.

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 2, 2024

8 Warnings
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/BookmarksViewController.swift:620:20: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/BookmarksViewController.swift:647:20: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/RootDebugViewController.swift:103:20: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'other_method' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/RootDebugViewController.swift:113:20: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'other_method' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/RootDebugViewController.swift:121:20: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'other_method' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/RootDebugViewController.swift:93:20: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'other_method' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/TabViewController.swift:974:5: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
⚠️ This PR introduced a violation in DuckDuckGo: /DuckDuckGo/TabViewController.swift:980:13: warning: Type Contents Order Violation: An 'ib_segue_action' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
21 Messages
📖 Linting Aerial with this PR took 0.91s vs 0.92s on main (1% faster)
📖 Linting Alamofire with this PR took 1.26s vs 1.26s on main (0% slower)
📖 Linting Brave with this PR took 7.17s vs 7.17s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 5.3s vs 5.32s on main (0% faster)
📖 Linting Firefox with this PR took 10.68s vs 10.76s on main (0% faster)
📖 Linting Kickstarter with this PR took 10.13s vs 10.14s on main (0% faster)
📖 Linting Moya with this PR took 0.53s vs 0.53s on main (0% slower)
📖 Linting NetNewsWire with this PR took 2.9s vs 2.9s on main (0% slower)
📖 Linting Nimble with this PR took 0.78s vs 0.78s on main (0% slower)
📖 Linting PocketCasts with this PR took 8.61s vs 8.58s on main (0% slower)
📖 Linting Quick with this PR took 0.44s vs 0.44s on main (0% slower)
📖 Linting Realm with this PR took 4.47s vs 4.49s on main (0% faster)
📖 Linting Sourcery with this PR took 2.31s vs 2.33s on main (0% faster)
📖 Linting Swift with this PR took 4.5s vs 4.5s on main (0% slower)
📖 Linting VLC with this PR took 1.25s vs 1.28s on main (2% faster)
📖 Linting Wire with this PR took 17.91s vs 17.89s on main (0% slower)
📖 Linting WordPress with this PR took 11.38s vs 11.37s on main (0% slower)
📖 This PR fixed a violation in DuckDuckGo: /DuckDuckGo/BookmarksViewController.swift:620:20: warning: Type Contents Order Violation: An 'other_method' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
📖 This PR fixed a violation in DuckDuckGo: /DuckDuckGo/BookmarksViewController.swift:647:20: warning: Type Contents Order Violation: An 'other_method' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
📖 This PR fixed a violation in DuckDuckGo: /DuckDuckGo/TabViewController.swift:974:5: warning: Type Contents Order Violation: An 'other_method' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)
📖 This PR fixed a violation in DuckDuckGo: /DuckDuckGo/TabViewController.swift:980:13: warning: Type Contents Order Violation: An 'other_method' should not be placed amongst the type content(s) 'instance_property' (type_contents_order)

Generated by 🚫 Danger

@dk-talks dk-talks closed this Apr 3, 2024
@dk-talks dk-talks deleted the dk-talks-add_TypeContent_ib_segue_action branch April 3, 2024 07:54
@dk-talks dk-talks restored the dk-talks-add_TypeContent_ib_segue_action branch April 3, 2024 07:56
@dk-talks dk-talks reopened this Apr 3, 2024
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.

Thanks! This needs some tests. See TypeContentsOrderRuleTests.

Also, you need to update the test reference for IntegrationTests.testDefaultConfigurations to make the build pass.

@dk-talks
Copy link
Contributor Author

dk-talks commented Apr 9, 2024

Hi, @SimplyDanny still the build is not passing, is there something else needed to be done?

@SimplyDanny
Copy link
Collaborator

Hi, @SimplyDanny still the build is not passing, is there something else needed to be done?

Now that you have removed the entry from the default list, the test reference update needs to be reverted.

@dk-talks
Copy link
Contributor Author

dk-talks commented Apr 9, 2024

Hey @SimplyDanny. Now that the build has passed, is there anything else needs to be done or is it enough to merge and close the issue?

@SimplyDanny
Copy link
Collaborator

Hey @SimplyDanny. Now that the build has passed, is there anything else needs to be done or is it enough to merge and close the issue?

  1. This change needs tests. See TypeContentsOrderRuleTests.
  2. A changelog entry needs to be added. See CHANGELOG.md.

@SimplyDanny SimplyDanny force-pushed the dk-talks-add_TypeContent_ib_segue_action branch from e79276a to d109056 Compare December 14, 2024 17:23
@SimplyDanny SimplyDanny enabled auto-merge (squash) December 14, 2024 17:25
@SimplyDanny SimplyDanny force-pushed the dk-talks-add_TypeContent_ib_segue_action branch from d109056 to be4b489 Compare December 14, 2024 22:41
@SimplyDanny SimplyDanny merged commit 8e3b50f into realm:main Dec 14, 2024
14 checks passed
@guillaumealgis
Copy link

guillaumealgis commented Jan 13, 2025

Hi, thanks for adding this new case to the type_contents_order rule :)

It seems the documentation has not been updated, and I could not find where ib_segue_action is located in the default configuration for this rule, making it difficult to know where SwiftLint expects me to put my @IBSegueAction methods 😕

EDIT: It seems that putting the @IBSegueAction methods at the end of the enclosing type's implementation makes SwiftLint happy.

@SimplyDanny
Copy link
Collaborator

Hi, thanks for adding this new case to the type_contents_order rule :)

It seems the documentation has not been updated, and I could not find where ib_segue_action is located in the default configuration for this rule, making it difficult to know where SwiftLint expects me to put my @IBSegueAction methods 😕

EDIT: It seems that putting the @IBSegueAction methods at the end of the enclosing type's implementation makes SwiftLint happy.

We intentionally didn't specify a default expectation because the new type was added on top of the already existing ones. And what's not explicitly specified is expected last by the rule.

@guillaumealgis: Do you think a default would be reasonable (also for documentation purposes) instead of leaving it unspecified? If so, what would be a decent order? Is putting it on the same level as @IBAction a good idea?

@guillaumealgis
Copy link

Do you think a default would be reasonable (also for documentation purposes) instead of leaving it unspecified?

I think SwiftLint should explicitly define an order for all defined cases. This makes the expected position of the elements clearer for everyone.

I have honestly no strong opinion on where to put ib_segue_action in the list. Same level / right after ib_action seems fine, between other_method and subscript would be fine also, end of list is not outrageous 🤷🏼‍♂️

what's not explicitly specified is expected last by the rule

Seems pretty logical, but it's not documented, so when SwiftLint started complaining after I updated it was not clear where I should put my code to satisfy it :)

@SimplyDanny
Copy link
Collaborator

I have honestly no strong opinion on where to put ib_segue_action in the list. Same level / right after ib_action seems fine, between other_method and subscript would be fine also, end of list is not outrageous 🤷🏼‍♂️

Same level as ib_action should be fine then: #5956.

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.

5 participants