Named sessions#8
Conversation
| * @param debugName An identifying name | ||
| * @return A new instance of `RemoteView` | ||
| */ | ||
| def dill(debugName: String): RemoteView = RemoteView.dill(defaultAddress, Some(debugName)) |
There was a problem hiding this comment.
This is a bit sus: there would be ambiguity with the method below if I called dill("foo"), so I think we need to be a bit more careful. I'm unsure exactly how though, thoughts?
There was a problem hiding this comment.
Had a bad feeling about that. Could use builder methods?
There was a problem hiding this comment.
Doesn't feel particularly idiomatic... I think the debugName is the "more important" of the two, right? Perhaps we could just make dill and dillWithAddress or something?
j-mie6
left a comment
j-mie6
left a comment
There was a problem hiding this comment.
Ah I see, yeah this is probably fine. However, I would probably make it a setName method to make it clear it alters it... I'd prefer immutable with case class, but then the sessionId is already a var.
|
Isn't that a bit more verbose for a user by forcing multiple lines of stmts? val view = RemoteView.dill
view.setName("..")
parser.attach(view) |
|
True, but the mutation irks me a little. Perhaps we could make it do a proper copy. The issue I have is with the sessionId, which is a true variable... |
|
I suppose that |
|
Maybe an alternative to get sessionId as val- Add another app endpoint dedicated for id provisioning, which is called when constructing a RemoteView and so the returned id can be immutably set |
|
Not a bad solution... that way we can switch to |
| implicit val responsePayloadRW: up.ReadWriter[NewSessionResponse] = up.macroRW[NewSessionResponse] | ||
|
|
||
| val response: Try[Response[Either[ResponseException[String, Exception], NewSessionResponse]]] = basicRequest | ||
| .readTimeout(ConnectionTimeout) |
There was a problem hiding this comment.
I assume these four lines are the same for all endpoint calls, could we factor them out into a dillRequest endpoint builder? then dillRequest.post(newSessionEndpoint).response...
There was a problem hiding this comment.
Not named as dillRequest since it's 'generic' to Remotes but yes 👍
| // Identifies a session for the receiver | ||
| private var sessionId: Int = -1 | ||
|
|
||
| case class RemoteView private (protected val port: Int, protected val address: String, protected val debugName: Option[String] = None) extends DebugView.Reusable with DebugView.Pauseable with DebugView.Manageable { |
There was a problem hiding this comment.
should be final, shouldn't need protected val, just write port: Int, etc
| def apply(userPort: Int = defaultPort, userAddress: String = defaultAddress, debugName: Option[String] = None): RemoteView = { | ||
| require(userPort <= MaxUserPort, s"Remote View port invalid : $userPort > $MaxUserPort") | ||
| require(checkIp(userAddress), s"Remote View address invalid : $userAddress") | ||
| require(debugName.forall(!_.isEmpty), "debugName should not be empty") |
There was a problem hiding this comment.
Oh lol I was searching for _.isNonEmpty 😞
| * @param skipBreakpoint How many breakpoints to skip after this breakpoint (not required). | ||
| */ | ||
| private [debug] case class RemoteViewResponse(message: String, sessionId: Int, skipBreakpoint: Int = -1, newRefs: Seq[CodedRef] = Nil) | ||
| private [debug] case class RemoteViewResponse(message: String, skipBreakpoint: Int = -1, newRefs: Seq[CodedRef] = Nil) |
|
I guess we now need this to be implemented DILL side and released asap? |
|
Yepp |
2 participants
RemoteViewThen needs picking up on debug-app side with serde.