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

Rename an API that has the same name as a new API in iOS SDK #1400

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

crazytonyli
Copy link
Contributor

iOS 18 SDK has a new API NSAttributedString.init(attachment:attributes:). This library has a public API that has the same name.

This PR did a quick fix which adds a meaning less aztec: Void argument to the API, so that the library get to keep the existing functionality and won't affect library consumers in using iOS 18 SDK.


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@@ -11,14 +11,14 @@ class UnknownEditorViewController: UIViewController {
///
fileprivate(set) var saveButton: UIBarButtonItem = {
let saveTitle = NSLocalizedString("Save", comment: "Save Action")
return UIBarButtonItem(title: saveTitle, style: .plain, target: self, action: #selector(saveWasPressed))
return UIBarButtonItem(title: saveTitle, style: .plain, target: UnknownEditorViewController.self, action: #selector(saveWasPressed))
Copy link

Choose a reason for hiding this comment

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

This doesn't seem right 🤔
It's now setting a class as a target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! These are Xcode's code suggestions, which I blindly took... Addressed in fc8861a.

/// Helper Initializer: returns an Attributed String, with the specified attachment, styled with a given
/// collection of attributes.
///
public convenience init(attachment: NSTextAttachment, attributes: [NSAttributedString.Key: Any], aztec: Void) {
Copy link

Choose a reason for hiding this comment

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

(nit) I would suggest converting it to a factory method with a prefix: NSAttributedString.aztec_make(attachment:attributes:). Does it need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This initialiser is available to both NSAttributedString and NSMutableAttributedString. It's not very straightforward to make a static function to return an instance of its receiver.

Does it need to be public?

I'm not sure. But I don't want to change the public API as part of this PR though, which is why I marked the original function as deprecated.

@crazytonyli crazytonyli requested a review from kean August 15, 2024 23:56
@crazytonyli
Copy link
Contributor Author

The unit test failures are not related to this PR. #1392 seems to fix them.

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