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

v2.0 #30

Merged
merged 4 commits into from Sep 2, 2016
Merged

v2.0 #30

merged 4 commits into from Sep 2, 2016

Conversation

kalaninja
Copy link
Collaborator

Hi there,
I've managed to deal with all my "busy man" problems. So, back online and ready for some opensource.

Here is the PR for v2.0. It doesn't have a readme file yet, because I am unaware about the links to godoc, travis, etc. I assume that if we tag the new commit with as "v2.0", the link to godoc would be smth like
https://godoc.org/gopkg.in/ahmetalpbalkan/go-linq.v2.0 and it seems that tha branch for travis and coveralls can be specified at their urls. Is it correct? Should I tag it myself?

@ahmetb
Copy link
Owner

ahmetb commented Aug 29, 2016

@kalaninja thanks for this PR! Don't worry about README etc for now, we can address the docs/website issues later.

Do you mind giving a brief overview of how much exported function/type signatures have changed? That would help me to review and give an idea of how much of a breaking change it is.

@@ -1,14 +1,21 @@
Copyright 2013 Ahmet Alp Balkan
The MIT License (MIT)
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this one is a leftover. Can you please revert this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Didn't notice that.

@kalaninja
Copy link
Collaborator Author

About an overview it seems to be a good idea? How do you want to have that info? I may do it as a table in excel or google docs with my methods on one side and yours on the other? Or you might have a better idea?

@ahmetb
Copy link
Owner

ahmetb commented Aug 30, 2016

@kalaninja actually, I think I got the main idea. There are quite many new methods, some methods are gone, errors are gone, linq.T is now interface{} etc.

I also found godocs to be helpful. How about you add a README.md (by updating the old one?) in the meanwhile and I give you commit access after merging this. The remaining tasks would be:

  • we should probably add a big notice to the README.md saying v2.0 is a breaking change, for those who want, we can link to a v1.0 branch
    • for that we first need to tag a v1.0 branch :) this can be done later.
  • update README with new examples
  • update Wiki tab with new examples (or maybe get rid of the wiki altogether, WDYT?)
  • update the gh-pages branch for the website with new examples

@kalaninja
Copy link
Collaborator Author

Yes, I don't have error because they made the predicates look ugly, and its effect can be achieved with panic and recover.
Yes, I used interface{} instead of linq.T, but now I am unsure about that decision, probably usage of linq.T made the code look prettier. WDYT?

I'll do the readme asap. Agree with the todo list. And I do not believe that the wiki is really helpful (I didn't even know that you have one)) You have a beautiful site and we might invest more effort there.

// func (f foo) CompareTo(c Comparable) int {
// a, b := f.f1, c.(foo).f1
//
// if a < b {
Copy link
Owner

@ahmetb ahmetb Aug 30, 2016

Choose a reason for hiding this comment

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

this code can be shorter as?:

switch{
case a>b: return -1
case b<a: return 1
default: return 0
}

Copy link
Collaborator Author

@kalaninja kalaninja Aug 31, 2016

Choose a reason for hiding this comment

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

they seem pretty the same legth since I use gofmt

            switch {
            case a > b:
                return -1
            case b < a:
                return 1
            default:
                return 0
            }

vs

            if a < b {
                return -1
            } else if a > b {
                return 1
            }

            return 0

There are other variants as well. Consider:

            if a < b {
                return -1
            }
            if a > b {
                return 1
            }
            return 0

WDYT?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess it doesn't matter too much then. I still prefer

    switch {
    case a > b:
        return -1
    case b < a:
        return 1
    default:
        return 0
    }

mostly because it's easier to follow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. It looks really straightforward. I'll change that.

@ahmetb
Copy link
Owner

ahmetb commented Aug 31, 2016

@kalaninja This generally looks good to me. Can you just add a README by copying from the current one and updating the example etc. Then it would be good to merge. Then we can address other stuff in #30 (comment).

@kalaninja
Copy link
Collaborator Author

I've attached the readme, changed "if" to "switch" and removed the book struct from the example.

}).First()
```

**More examples** can be found in [documentation](https://godoc.org/gopkg.in/ahmetalpbalkan/go-linq.v2)
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe don't link to these like v2 yet because we'll merge it to master.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And what will happen with the current version?

Copy link
Owner

Choose a reason for hiding this comment

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

I archived the current version in archive/0.9 branch, anybody who wants to use it I guess can grab it from there or perhaps grab it via go get gopkg.in/... (we have a v0.9-rc5 tag, we can tag it as v0.9 anytime if that'll make it work with gopkg.in).

@kalaninja
Copy link
Collaborator Author

I've fixed links in readme, removed the line about plinq, recovered the one about vanilla go, added release notes.
I've tried several ways to make the example more readable and ended with renaming a to author, b to book and g to group. I've added comments as well.

@ahmetb
Copy link
Owner

ahmetb commented Sep 2, 2016

@kalaninja thanks! I'll open issues for the rest of the updates we probably need to make.

@ahmetb ahmetb merged commit d1ca28a into ahmetb:master Sep 2, 2016
@ahmetb
Copy link
Owner

ahmetb commented Sep 2, 2016

💥 🎊 🍷🍾

@kalaninja kalaninja deleted the v2.0 branch September 2, 2016 22:14
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