-
Notifications
You must be signed in to change notification settings - Fork 99
Add new parameter for date formats, and default to en_US #287
Conversation
@@ -2,6 +2,7 @@ import SwiftSemantics | |||
import SwiftDoc | |||
import CommonMarkBuilder | |||
import HypertextLiteral | |||
import struct Foundation.Locale |
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.
In some files the compiler was having some issues with Protocol
once Foundation was imported, also when I used SwiftSemantics.Protocol
instead of just Protocol
, so I've imported only Foundation.Locale
where the issue was happening
@@ -72,11 +78,11 @@ extension SwiftDoc { | |||
for symbol in module.interface.topLevelSymbols.filter(symbolFilter) { | |||
switch symbol.api { | |||
case is Class, is Enumeration, is Structure, is Protocol: | |||
pages[route(for: symbol)] = TypePage(module: module, symbol: symbol, baseURL: baseURL, includingChildren: symbolFilter) | |||
pages[route(for: symbol)] = TypePage(module: module, symbol: symbol, baseURL: baseURL, datesLocale: datesLocale, includingChildren: symbolFilter) |
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.
Please avoid long lines, as with these changes it now requires horizontal scrolling to review even on a wide screen, and makes it even harder on narrow screens. This is applicable to the rest of the long lines in this diff.
pages[route(for: symbol)] = TypePage(module: module, symbol: symbol, baseURL: baseURL, datesLocale: datesLocale, includingChildren: symbolFilter) | |
pages[route(for: symbol)] = TypePage( | |
module: module, | |
symbol: symbol, | |
baseURL: baseURL, | |
datesLocale: datesLocale, | |
includingChildren: symbolFilter | |
) |
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.
Would be cool to some swiftformat and swiftlint config to automate the check and possibly also the formatting part :P
What do you think?
I can help to set it up if you want
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.
Thank you, sounds good to me! @mattt WDYT?
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.
If we are going to add linting, I would suggest that we should stick to swift-format for linting rather than using swift-lint. As to setting that up, its pretty trivial (there is an example at https://github.com/compnerd/swift-win32/blob/main/.github/workflows/lint.yml).
Co-authored-by: Max Desiatov <max@desiatov.com>
@@ -37,6 +40,8 @@ struct FooterPage: Page { | |||
} | |||
|
|||
var html: HypertextLiteral.HTML { | |||
dateFormatter.locale = datesLocale |
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've also noticed that there is a discrepancy with the Markdown footer, that uses "Generated at \(timestamp) using [swift-doc](\(href)) \(SwiftDoc.configuration.version)."
while it could use "Generated on \(dateString)"
instead.
WDYT?
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.
Hello everyone 馃憢馃徎
First of all, thank you for this great tool!
While I was generating some documentation I've noticed that it was using my locale, which makes the result odd, because uses an italian word in the middle of an english sentence.
I've then added a new option (that defaults to
en_US
) to specify the locale for the dates that are printed by swift docs.I could just hardcode
en_US
there, but I thought that making it explicit with an option and allow the user to modify it was a better choice.And now is correct also with my italian locale