Skip to content

Implement net.Conn high-level interface, improve low-level interface. #3

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

Merged
merged 39 commits into from
Jan 11, 2015

Conversation

mjohnson9
Copy link
Member

This is for discussion about merging the new wrappers into master.

Previous discussion was in #2.

Michael Johnson added 30 commits October 27, 2014 14:53
Better console messages
Better domain detection
A second string echo test
A binary echo test
... as per code review in #1
... as per code review in #1
@dmitshur
Copy link
Member

dmitshur commented Dec 7, 2014

I see. https://github.com/dominikh/go-js-xhr does not seem to have any.

@dominikh
Copy link
Member

dominikh commented Dec 7, 2014

My packages really aren't a good indicator for testability, I'm not big on tests in general (one of my weaknesses.)

@dmitshur
Copy link
Member

What remains to be done before this can be merged?

I've tried out both new APIs over at shurcooL/play@9b27257 and it seems good to me.

I think once this is ready, we should merge it as is, and then work on taking positive improvements in #4 (but not all changes) and merging them into master.

@mjohnson9
Copy link
Member Author

Aside from exploring some better unit tests, I don't believe there's anything left.

dmitshur added a commit to shurcooL/play that referenced this pull request Dec 20, 2014
@dmitshur
Copy link
Member

What about documenting what events can be AddEventListener'ed and providing a short typical usage example as part of the package docs (for the WebSocket API)? Is that already done?

@mjohnson9
Copy link
Member Author

Indeed. They're documented as part of the package docs.

@dmitshur
Copy link
Member

Looks great. I've updated the README to match, does that look all right?

I think we can merge this soon, yeah?

Oh, we should probably mention the license (MIT, something permissive just in case someone needs to ask). And work on adding support for Travis so tests can be run there.

@dmitshur dmitshur changed the title New interface Implement net.Conn high-level interface, improve low-level interface. Dec 20, 2014
@mjohnson9
Copy link
Member Author

I believe this is ready to merge. Before we do so, we may want to commit to a versioning standard (such as semver) and use Git tags (perhaps in the format proposed by gopkg.in?) to mark versions.

@dmitshur
Copy link
Member

Yeah, I'm okay to try using gopkg.in. This will be my first time trying it, but I think it's mostly additive and doesn't take anything away.

So, we can make the original (current master) version into branch v0, the new version (current dev) into branch v1. And separate point releases can be tags? Is that how it goes?

@mjohnson9
Copy link
Member Author

Yeah. We'd tag the latest commit on the master branch as v0. If we feel that it's going to be stable after this merge, we tag that commit as v1. Then, any changes that don't break compatibility will be v1.1 (or v1.0.1, as appropriate per semver).

@dmitshur
Copy link
Member

dmitshur commented Jan 4, 2015

When is a good time to wrap up this work?

I think we can merge this PR into master first, and treat it like v0. Then we incorporate improvements from #4. Then give it a week or two, and if there's nothing outstanding, we tag it as v1.

I'm still not quite sure on how to use branches/tags for this gopkg.in stuff, since I don't have experience, and the only way to have experience is to start trying it.

master branch (default branch?) - latest development version.
v0 tag (or branch?) - pretty useless, but it can be a "release candidate" for v1.
v1 tag (or branch?) - first official stable release, to be made when things are good on master?
v1.0.1 tag - first patch, etc.
v1.1 tag - first minor update, etc.
v2 tag - first major breaking API change.

Just... thinking this through.

Edit: I've seen that v1 and v2 and master are all branches on some projects, rather than v1 and v2 being tags. How does that work? E.g., see https://github.com/go-pipe/pipe/branches. (/cc @niemeyer if you have some spare cycles to offer tips/hints here.)

Edit 2: After talking with @slimsag, I got a better understanding of how gopkg.in should work. I think the plan should be to keep master as the latest development version, and create v1, v1.0.1, v1.1, v2, etc. tags when we decide to explicitly release those versions.

@dmitshur
Copy link
Member

dmitshur commented Jan 9, 2015

@nightexcessive, are you okay with using MIT license for this?

@mjohnson9
Copy link
Member Author

I'd prefer the BSD 3-Clause, primarily for its protections of the contributors' names.

@dmitshur
Copy link
Member

dmitshur commented Jan 9, 2015

Let's go with your preference, you wrote most of the new implementation. I am indifferent (and uneducated about the differences), as long as the license is permissive.

@mjohnson9
Copy link
Member Author

Awesome.

I'm no legal expert, but I believe they're almost the same sans the contributor name protections. 👍

@dmitshur
Copy link
Member

dmitshur commented Jan 9, 2015

I would ask what "contributor name protection" means, but that would imply I care, which I don't think I do, so I will not ask.

Yes, this is me not asking what it means. :P

@dmitshur
Copy link
Member

dmitshur commented Jan 9, 2015

I think we should add that license, and merge this PR into master. We can then create a new issue about considering switching to gopkg.in as the primary import path and discuss that there. I think the issue is still somewhat complicated so I'd prefer to separate it from this PR and the other open PR we have to close first.

@mjohnson9
Copy link
Member Author

Sounds good!

@dmitshur
Copy link
Member

@nightexcessive, anything you're waiting on? Can I help? I'd like to move forward on this whenever you are ready. :)

@mjohnson9
Copy link
Member Author

I'm not waiting on anything. I'll go ahead and merge this now!

mjohnson9 pushed a commit that referenced this pull request Jan 11, 2015
Implement net.Conn high-level interface, improve low-level interface.
@mjohnson9 mjohnson9 merged commit edb073b into master Jan 11, 2015
@mjohnson9
Copy link
Member Author

All merged.

@dmitshur
Copy link
Member

Excellent! Thank you! And congratulations! :)

@dmitshur dmitshur deleted the dev branch January 11, 2015 01:27
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.

4 participants