Skip to content
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 withDebuggerName #60

Open
wants to merge 1 commit into
base: v4.x
Choose a base branch
from
Open

add withDebuggerName #60

wants to merge 1 commit into from

Conversation

OnurGumus
Copy link

Fiexes Allow specifying 'name' #58

@@ -41,10 +41,11 @@ module Debugger =
hostname = "remotedev.io"
port = 443
secure = true
name = "Elmish"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic string "Elmish" used in several places. Here, L141, and L163. Would empty string be a better default?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well one is for fallback the other is the default. I am not sure, what is best here though.

try
let deflater, inflater = getTransformers<'model>()
let connection = Debugger.connect<'msg> (Debugger.ViaExtension name)
withDebuggerUsing deflater inflater connection program

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the helper functions call withDebuggerUsing. Maybe the name parameter should be there? To avoid a breaking change, could introduce a new fn withNamedDebuggerUsing with name parameter, move current withDebuggerUsing code there. Then change withDebuggerUsing body to just call withNamedDebuggerUsing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@et1975 Thoughts?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also reduce the usage of the default name down to 1 place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using says take this connection, at which point, it seems, specifying the name is already too late. Guess we could change the signature of Using to take a function: name -> connection, but that would change the assumptions I originally had when writing this - someone could establish the connection in an arbitrary fashion with regards to options and timing.
I'm onboard with DRY though, there must be a better way to express the default - once.

Copy link

@kspeakman kspeakman Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could name be string option? Looks like Thoth's default behavior is to skip null fields. When name is None, it should emit the same json as before this change. Or we could default it to "Elmish" before connecting. Then there's no need for an extra Using fn.

Copy link

@kspeakman kspeakman Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe it should be ViaExtension and ViaNamedExtension of name: string. Then no need to worry about default in other fns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to worry about default in other fns.

I was wondering about that, @OnurGumus is naming supported with remote debugger?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not; but I am not sure.

@OnurGumus
Copy link
Author

Is this going to go in?

@et1975
Copy link
Member

et1975 commented Mar 13, 2023

Is this going to go in?

We have unresolved questions, are you planning to address them?

@OnurGumus
Copy link
Author

Can we summarise them? I lost a bit track on what else is required here?

@et1975
Copy link
Member

et1975 commented Mar 24, 2023

I believe the gist of the commentary so far is that we may need to reorg how defaults are applied so that they don't appear over and over again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants