-
Notifications
You must be signed in to change notification settings - Fork 6
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
Nimble: update to latest upstream and make it Xcode12 ready. #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Just one query about the changes to expect(expression:)
.
LGTM. Merge when ready. |
fde1d4e
to
83c06a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing a few things:
- What ambiguity are you trying to solve? An explanation and an example would be great.
- How does the the change in the code solve it? It's not immediately apparent from the code.
- There are commits that don't seem related to Xcode 12, for example removing deprecated APIs. Why are they here then?
@@ -1830,6 +1830,7 @@ | |||
CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF = YES; | |||
CLANG_WARN_OBJC_LITERAL_CONVERSION = YES; | |||
CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR; | |||
CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER = YES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the one recommended by Xcode. It was before I learned NO
is as good, but since it's working already no reason to change.
Note that upstream has now solved the ambiguity/crash issue and is XCode 12 ready Quick#824 |
Yep. I saw it. We'll advance our fork and I'll drop whatever I could from this PR. We don't want to diverge at all. But some upstream changes (like dropping deprecated Matcher API) we'd rather not wait, because they cause unnecessary noise. |
@barakwei please advance Nimble fork to v9.0.0-rc.3 as it incorporates ambiguous use of 'expect' fix I previously cherry picked. |
pulled master, good luck |
@byohay have a look. I removed details of "ambiguous expect" since it's not part of the PR anymore. I'll write about it on ++Nimble in Foundations. |
Can we ++Nimble even before this is merged? |
If we do, some apps will get |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more deprecated stuff:
Matcher
protocol.NMBObjCMatcher
expressionDoesNotMatch
postNotifications
_fromDeprecatedClosure
, but it seems to be deeply engrained inPredicate
.
What about them?
I tried removing those but it's tricky and has no real value. I surveyed several apps to find all the deprecated APIs that have negative impact. In addition I deleted all Matcher usages that were easy to delete just in case. |
@barakwei merge at your leisure, for some reason I can't. |
Matcher
protocol.Otherwise matching expect statement on an optional value often resulted in the following warning
'to(:description:)' is deprecated: User Predicate instead_.