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

Change acknowledgement access level to open #128

Merged

Conversation

cheshire0105
Copy link
Contributor

Title: Make acknowledgement Property Accessible for Subclassing

Problem:
The internal access level of AcknowViewController.acknowledgement blocks subclassing outside the module, causing 'acknowledgement' is inaccessible errors.

Solution:

  • Change acknowledgement’s access modifier to open.
  • Enable cross-module subclassing and customization.

Testing:

  • Created a subclass of AcknowViewController.
  • Validated property access and GitHub license fetching.

Benefits:

  • ✨ Enables advanced view controller customization.
  • ✅ Aligns with Swift’s access control guidelines.

Hi maintainers! 👋
This fix resolves a common subclassing limitation. Would love your feedback! 🙌

@vtourraine vtourraine merged commit f5b51a9 into vtourraine:main Jan 21, 2025
1 check passed
@vtourraine
Copy link
Owner

Hello @cheshire0105, and thank you so much for sending this pull request. It’s a small but very useful improvement. I’d be curious to see how you use this level of customization in your app!

@cheshire0105
Copy link
Contributor Author

Hello @cheshire0105, and thank you so much for sending this pull request. It’s a small but very useful improvement. I’d be curious to see how you use this level of customization in your app!

### **Code Modification Explanation**

---

#### **1. My Implementation (Problematic Code)**
```swift
// CustomAcknowViewController.swift
class CustomAcknowViewController: AcknowViewController {
    override func viewDidLoad() {
        super.viewDidLoad()
        
        // Attempting to fetch GitHub license
        if canFetchLicenseFromGitHub, 
           let repository = acknowledgement?.repository { // ❌ Error trigger
            fetchGitHubLicense(repository: repository)
        }
    }
}

2. Why This Code Was Needed

  • Core Purpose: Dynamically fetch license details from GitHub to enhance OSS attribution.
  • Critical Dependency: Required access to acknowledgement.repository (the package's GitHub URL).
  • Customization Goal: Display real-time license data beyond static text provided by the library.

3. The Error

'acknowledgement' is inaccessible due to 'internal' protection level
  • Root Cause:
    The original library code declared acknowledgement as internal:
    // Original AcknowList code
    internal var acknowledgement: Acknow? // 👉 Subclasses can't access this
  • Impact:
    • Blocked access to essential data (repository URL) in subclasses.
    • Broke GitHub license-fetching functionality in custom view controllers.

4. Why Modification Was Necessary

  • Developer Experience:
    // Without fix: Compiler blocks access
    let repo = acknowledgement.repository // ❌ "You shall not pass!" 
  • Extensibility Failure:
    A library promoting subclassing must expose critical properties.
  • Violation of Swift Conventions:
    An open class with internal properties creates a dead end for customization.

🔧 The Fix

// Modified AcknowList code
@MainActor open class AcknowViewController : UIViewController {
    open var acknowledgement: Acknow? // ✅ Now accessible
}

Key Changes

Aspect Before After
Access Level internal open
Subclass Access Denied Granted
Customization Restricted Fully Enabled

🚀 Post-Fix Behavior

// Now works flawlessly
if let repoURL = acknowledgement?.repository { // ✅ Perfect access
    fetchGitHubLicense(repository: repoURL) 
}
  • Restored Functionality: Dynamic license loading via GitHub API.
  • Ecosystem Impact: All developers using AcknowList gain customization freedom.
  • Code Safety: Optional chaining (?.) replaces risky force-unwrapping.

📌 Why This Matters

  1. Open/Closed Principle:
    • Libraries should be open for extension but closed for modification.
  2. Community Contribution:
    • Fixes a hidden barrier for others building on AcknowList.
  3. Future-Proofing:
    • Ensures compatibility with Swift’s strict access control philosophy.

🔍 Before vs. After

  • Before: Hacky workarounds (e.g., reflection) → Runtime crashes.
  • After: Native Swift syntax → Compile-time safety.

ℹ️ Note

I'm Korean, and I may not be very fluent in English. I apologize if any part of my explanation feels awkward or unclear. If there’s anything that needs improvement or clarification, please feel free to let me know. Thank you for your understanding, and I truly appreciate your feedback! Wishing you all the best!

저는 한국인이고 영어가 유창하지 않을 수 있습니다. 제 설명 중 어색하거나 명확하지 않은 부분이 있다면 양해 부탁드립니다. 개선이나 설명이 필요한 부분이 있다면 언제든지 말씀해 주세요. 이해해 주셔서 감사드리며, 소중한 피드백에 진심으로 감사드립니다! 항상 좋은 일만 가득하시길 바랍니다!


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.

2 participants