-
Notifications
You must be signed in to change notification settings - Fork 3
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
StringsDicts support #16
Conversation
@LukasHromadnik your pull request is missing a changelog! |
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 am also missing some tests regarding this feature 🤔
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.
Well right now I think we're just missing the tests 🙂
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.
two minor comments, otherwise seems good, can't wait for this to land !
} | ||
} | ||
|
||
class SheetsAPIServiceMock: SheetsAPIServicing { |
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.
just a nitpick, if ACKLocalizationPluralsTests
is final
I would also make this class final
import XCTest | ||
@testable import ACKLocalizationCore | ||
|
||
class AuthAPIServiceMock: AuthAPIServicing { |
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.
just a nitpick, if ACKLocalizationPluralsTests
is final
I would also make this class final
I made some changes to the plurals parser. Now we are able to determine if the translation key / plural key is missing, also all those checks throw an |
case invalidPluralRule(String) | ||
} | ||
|
||
extension PluralError: Equatable { |
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.
Equatable
should be automatically synthesized, shouldn't it?
LocRow(key: "##{zero}", value: "zero") | ||
] | ||
|
||
XCTAssertThrowsError(try ackLocalization.buildPlurals(from: rows)) { error in |
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.
might be nice to add a following extension for XCTest
(might be handy in the future, too):
func XCTAssertThrowsSpecific<Error: Swift.Error & Equatable, T>(_ closure: @autoclosure () throws -> T, _ error: Error, file: StaticString = #file, line: UInt = #line) {
do {
_ = try closure()
} catch let closureError as Error {
XCTAssertEqual(error, closureError, file: file, line: line)
} catch let closureError {
XCTFail("\(error) is not equal to: \(closureError)", file: file, line: line)
}
}
Your code can then be changed to something like this:
XCTAssertThrowsSpecific(try ackLocalization.buildPlurals(from: rows), PluralError.missingTranslationkey(rows[0].key))
In combination with SwiftGen 6.3 we are now able to support simple stringsDicts.
Currently we can handle only top level plurals. That means that nested plurals where multiple plural keys are used in one single translation key are not supported.