-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add --host
option to docc preview
#713
base: main
Are you sure you want to change the base?
Changes from all commits
6543d4b
2a1539a
f8be787
e4cc3c1
845db03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,7 @@ public final class PreviewAction: Action, RecreatingContext { | |
|
||
var logHandle = LogHandle.standardOutput | ||
|
||
let host: String | ||
let port: Int | ||
|
||
var convertAction: ConvertAction | ||
|
@@ -75,6 +76,7 @@ public final class PreviewAction: Action, RecreatingContext { | |
/// Creates a new preview action from the given parameters. | ||
/// | ||
/// - Parameters: | ||
/// - host: The host name used by the preview server. | ||
/// - port: The port number used by the preview server. | ||
/// - convertAction: The action used to convert the documentation bundle before preview. | ||
/// On macOS, this action will be reused to convert documentation each time the source is modified. | ||
|
@@ -84,6 +86,7 @@ public final class PreviewAction: Action, RecreatingContext { | |
/// is performed. | ||
/// - Throws: If an error is encountered while initializing the documentation context. | ||
public init( | ||
host: String, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like I commented here I think it's sufficient to provide a default value for this added parameter to remain source compatible. It won't be ABI compatible but we don't require that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. Ethan's PR #254 is removing parameters. So he deprecated the old and add a new one. |
||
port: Int, | ||
createConvertAction: @escaping () throws -> ConvertAction, | ||
workspace: DocumentationWorkspace = DocumentationWorkspace(), | ||
|
@@ -95,6 +98,7 @@ public final class PreviewAction: Action, RecreatingContext { | |
} | ||
|
||
// Initialize the action context. | ||
self.host = host | ||
self.port = port | ||
self.createConvertAction = createConvertAction | ||
self.convertAction = try createConvertAction() | ||
|
@@ -108,13 +112,14 @@ public final class PreviewAction: Action, RecreatingContext { | |
@available(*, deprecated, message: "TLS support has been removed.") | ||
public convenience init( | ||
tlsCertificateKey: URL?, tlsCertificateChain: URL?, serverUsername: String?, | ||
serverPassword: String?, port: Int, | ||
serverPassword: String?, host: String, port: Int, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This initializer is already deprecated, we should not change the signature |
||
createConvertAction: @escaping () throws -> ConvertAction, | ||
workspace: DocumentationWorkspace = DocumentationWorkspace(), | ||
context: DocumentationContext? = nil, | ||
printTemplatePath: Bool = true) throws | ||
{ | ||
try self.init( | ||
host: host, | ||
port: port, | ||
createConvertAction: createConvertAction, | ||
workspace: workspace, | ||
|
@@ -163,13 +168,13 @@ public final class PreviewAction: Action, RecreatingContext { | |
// Preview the output and monitor the source bundle for changes. | ||
do { | ||
print(String(repeating: "=", count: 40), to: &logHandle) | ||
if let previewURL = URL(string: "http://localhost:\(port)") { | ||
if let previewURL = URL(string: "http://\(host):\(port)") { | ||
print("Starting Local Preview Server", to: &logHandle) | ||
printPreviewAddresses(base: previewURL) | ||
print(String(repeating: "=", count: 40), to: &logHandle) | ||
} | ||
|
||
let to: PreviewServer.Bind = bindServerToSocketPath.map { .socket(path: $0) } ?? .localhost(port: port) | ||
let to: PreviewServer.Bind = bindServerToSocketPath.map { .socket(path: $0) } ?? .localhost(host: host, port: port) | ||
servers[serverIdentifier] = try PreviewServer(contentURL: convertAction.targetDirectory, bindTo: to, logHandle: &logHandle) | ||
|
||
// When the user stops docc - stop the preview server first before exiting. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -201,7 +201,7 @@ class PreviewServerTests { | |
} | ||
|
||
func testPreviewServerBindDescription() { | ||
let localhostBind = PreviewServer.Bind.localhost(port: 1234) | ||
let localhostBind = PreviewServer.Bind.localhost(host: "localhost", port: 1234) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also test that by default, "localhost" is the host? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s the default in the sense that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would be good to test that |
||
XCTAssertEqual("\(localhostBind)", "localhost:1234") | ||
let socketBind = PreviewServer.Bind.socket(path: "/tmp/file.sock") | ||
XCTAssertEqual("\(socketBind)", "/tmp/file.sock") | ||
|
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.
Since this is a public initializer, providing a default value here would avoid a source breaking change for anyone who imports SourceDocCUtilities.