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

[WIP] Websockets support (fixes #22) #57

Closed
wants to merge 6 commits into from

Conversation

dlsniper
Copy link
Contributor

@dlsniper dlsniper commented Feb 4, 2015

Hi,

I've had a chat earlier with @tarqe39 and he had the idea that to do something to the first commit in this PR.
I'm very open to discussion and suggestions on how to move this forward.

I guess the next steps are:

  • get the output from cmd as currently the API is type cmdfunc func(proc *proctl.DebuggedProcess, args ...string) error (maybe add a *string/[]byte next to error?)
  • see if we need to wrap the output to json
  • see if we need to wrap the input to json
  • add tests? (looking for volunteers :D)
  • have this merged

P.S.
It took a while but I've manage to get some time off and now I can work a bit on getting this up and running. Sorry for delay.
If you want to change something, and have the credits for the change in the git history, send a PR to my fork, I have all the intentions to get this merged when it's in a good state.

Review on Reviewable

@dlsniper dlsniper changed the title Websockets support (fixes #22) [WIP] Websockets support (fixes #22) Feb 4, 2015
@derekparker
Copy link
Member

Awesome, I'm excited to see some work on this.

I think it could be interesting for a command to return maybe some sort of result struct and an error. The result struct could then respond to .String and .MarshalJSON, which would be suitable for any front end we use.

@dlsniper
Copy link
Contributor Author

dlsniper commented Feb 6, 2015

Cool, I'll do some work on this over the weekend and submit it in this PR as a different commit.

@dlsniper dlsniper force-pushed the add-websocket-support branch 4 times, most recently from 096545f to 4267e29 Compare February 9, 2015 16:55
@dlsniper
Copy link
Contributor Author

dlsniper commented Feb 9, 2015

Based on the ideas from @tarqe39, I've did some more refactoring on the initial PR in order to move it along the way it's described in the above description.

As a small observation, since goreadline is not using goimports to format their code, it's pretty ugly to always run goimports -w ./.. then revert that one file. Shouldn't it be always part of the repository?

The next step is to add a common command output structure to the core then format the output to the web and CLI as discussed above.

@derekparker
Copy link
Member

@dlsniper sorry for not getting back to you for awhile, been kind of heads down with OS X support lately.

I definitely think the goreadline sniped package should have correctly formatted imports, I might just push up a patch that fixes that formatting.

I've taken another brief look at your branch and it's looking good - definitely let me know when you'd like me to take another more in depth look at it.

@dlsniper dlsniper force-pushed the add-websocket-support branch 2 times, most recently from 2bb2bac to 8c8a8fb Compare February 14, 2015 14:57
@dlsniper
Copy link
Contributor Author

@derekparker don't worry, I don't really have time during the week anyway.
I've finished the adding support for output collection: 8c8a8fb
There are a few todos that I'd like you to have a look at as I'm not sure what's the correct way to solve them (or if they are correct to begin with, might be bugs?).

I've checked that the cli version works but as it is right now the web version doesn't work just yet.

Also, I'm not sure I'm happy with the way that we have the output right now.
I'm thinking to change the output struct from

type cmdOutput struct {
    Out string
    Err error
}

to something like

type cmdOutput struct {
    out interface{}
    err error
}

func (output *cmdOutput) JSONOutput() (string, error) {}
func (output *cmdOutput) Output() (string, error) {}
func (output *cmdOutput) Err() error {}

And then have the cmdOutput.out be decoded by the type of output in the respective output functions. What do you think?

@dlsniper dlsniper force-pushed the add-websocket-support branch 2 times, most recently from bd8fbe4 to 6ba0739 Compare February 14, 2015 20:37
@dlsniper dlsniper force-pushed the add-websocket-support branch 2 times, most recently from 33b7202 to f58296c Compare February 14, 2015 23:05
@dlsniper
Copy link
Contributor Author

I've fixed the web version as well and rebased on latest master. Aside from this, the concerns from above are still valid. Once delve can distinguish between output on console vs json, I believe this can be merged (ofc, with the post-feedback changes included as well)

@derekparker
Copy link
Member

@dlsniper I'll be taking a close look at this PR today and work towards helping move it along. Now that OS X support is merged in it's freed me up a lot.

@dlsniper
Copy link
Contributor Author

dlsniper commented Mar 3, 2015

@derekparker thanks a lot and congrats for the OS X support. I've put this on hold as I wanted to have some feedback with you before doing further changes.
Let me know if I should rebase this before you have a look at it :)

@derekparker
Copy link
Member

@dlsniper that would be great if you could rebase it.

@dlsniper
Copy link
Contributor Author

dlsniper commented Mar 3, 2015

Ok, I'll look over the effort I need to but it should be done in a couple of days. Btw, you can always ping me on: https://gitter.im/go-lang-plugin-org/go-lang-idea-plugin (I can't add you to the org so that we can chat in the private chan, but you can also drop me a line on Gitter directly).

@dlsniper
Copy link
Contributor Author

I've tried to rebase the PR for the past 2 hours but I've bumped into lots of internal changes and at this point I'll just close this and let someone else take care of this feature as I don't have the time required for it.

@dlsniper dlsniper closed this Mar 20, 2015
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.

None yet

2 participants