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

Introduce go modules #985

Merged
merged 6 commits into from Aug 27, 2018

Conversation

Projects
None yet
6 participants
@marwan-at-work
Contributor

marwan-at-work commented Aug 26, 2018

This PR is generated by a tool I wrote that helps migrate Go packages that are tagged +2 to Semantic Import Versioning.

Note that this change should be backwards compatible and can still be imported without the /vX suffix when Go Modules is off.

@googlebot googlebot added the cla: yes label Aug 26, 2018

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Aug 27, 2018

Collaborator

Thank you, @marwan-at-work!

Can you please figure out why Travis CI thinks that gofmt wasn't run on one or more of the files in this PR?

Collaborator

gmlewis commented Aug 27, 2018

Thank you, @marwan-at-work!

Can you please figure out why Travis CI thinks that gofmt wasn't run on one or more of the files in this PR?

marwan-at-work added some commits Aug 27, 2018

@marwan-at-work

This comment has been minimized.

Show comment
Hide comment
@marwan-at-work

marwan-at-work Aug 27, 2018

Contributor

@gmlewis seems that gofmt 1.11 has different rules from 1.10 and 1.9?

We can put a space between the longer field to make both gofmt's happy, but I wonder if this is worth investigating before "just making it work"

Contributor

marwan-at-work commented Aug 27, 2018

@gmlewis seems that gofmt 1.11 has different rules from 1.10 and 1.9?

We can put a space between the longer field to make both gofmt's happy, but I wonder if this is worth investigating before "just making it work"

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Aug 27, 2018

Collaborator

Thanks for fixing the Travis CI build, @marwan-at-work!

Since we recently removed support for Go 1.7 and bumped the version number, and this PR now removes support for Go 1.8, can you please make this semantic version be v18.0.0 and we will tag it as such when we merge the PR?

Also, can you please make the necessary change to README.md similar to the one I did here: fbd7fb9
?

Collaborator

gmlewis commented Aug 27, 2018

Thanks for fixing the Travis CI build, @marwan-at-work!

Since we recently removed support for Go 1.7 and bumped the version number, and this PR now removes support for Go 1.8, can you please make this semantic version be v18.0.0 and we will tag it as such when we merge the PR?

Also, can you please make the necessary change to README.md similar to the one I did here: fbd7fb9
?

@gmlewis

After the requested v18.0.0 change and the change to README.md, this LGTM.

Awaiting second LGTM from @juliaferraioli before merging.

@gmlewis gmlewis requested a review from juliaferraioli Aug 27, 2018

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Aug 27, 2018

Collaborator

To answer your question about investigating differences in gofmt between v1.11 and v1.10, it seems pretty minor and I'm fine to move forward keeping the builds happy, but if you would like to investigate, you are welcome to, obviously.

Collaborator

gmlewis commented Aug 27, 2018

To answer your question about investigating differences in gofmt between v1.11 and v1.10, it seems pretty minor and I'm fine to move forward keeping the builds happy, but if you would like to investigate, you are welcome to, obviously.

@juliaferraioli

Thanks @marwan-at-work! LGTM.

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Aug 27, 2018

Collaborator

Thank you, @juliaferraioli!
Merging then bumping tagged release.

Collaborator

gmlewis commented Aug 27, 2018

Thank you, @juliaferraioli!
Merging then bumping tagged release.

@gmlewis gmlewis merged commit 08dbadc into google:master Aug 27, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marwan-at-work

This comment has been minimized.

Show comment
Hide comment
@marwan-at-work

marwan-at-work Aug 27, 2018

Contributor

Thank you both :)

Contributor

marwan-at-work commented Aug 27, 2018

Thank you both :)

@gmlewis gmlewis referenced this pull request Aug 30, 2018

Closed

remove v18 postfix #987

@tmc

This comment has been minimized.

Show comment
Hide comment
@tmc

tmc Aug 31, 2018

I think github.com/google/go-github/github/v18 in the readme is supposed to be github.com/google/go-github/v18/github.

tmc commented Aug 31, 2018

I think github.com/google/go-github/github/v18 in the readme is supposed to be github.com/google/go-github/v18/github.

@tmc

This comment has been minimized.

Show comment
Hide comment
@tmc

tmc Aug 31, 2018

Perhaps the README should contain instructions for pre-go1.11 users as well?

tmc commented Aug 31, 2018

Perhaps the README should contain instructions for pre-go1.11 users as well?

@marwan-at-work

This comment has been minimized.

Show comment
Hide comment
@marwan-at-work

marwan-at-work Aug 31, 2018

Contributor

@tmc yes that was a typo on my end, will push a fix. Thanks for the callout!

Contributor

marwan-at-work commented Aug 31, 2018

@tmc yes that was a typo on my end, will push a fix. Thanks for the callout!

@marwan-at-work

This comment has been minimized.

Show comment
Hide comment
@marwan-at-work

marwan-at-work Aug 31, 2018

Contributor

As for pre go 1.11, the latest Go 1.9 and 1.10 should work fine with the new import path. However they wouldn't work with Go 1.8 or before that. So before 1.8, they can just use v17 or below. But maybe the readme should just say the package requires Go1.9.X or above?

Contributor

marwan-at-work commented Aug 31, 2018

As for pre go 1.11, the latest Go 1.9 and 1.10 should work fine with the new import path. However they wouldn't work with Go 1.8 or before that. So before 1.8, they can just use v17 or below. But maybe the readme should just say the package requires Go1.9.X or above?

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Aug 31, 2018

Collaborator

...and it does:

go-github requires Go version 1.9 or greater.

Collaborator

gmlewis commented Aug 31, 2018

...and it does:

go-github requires Go version 1.9 or greater.

- "1.10.x"
- "1.9.x"
- "1.8.x"
- master
- tip

This comment has been minimized.

@dmitshur

dmitshur Sep 8, 2018

Member

@marwan-at-work Can you please help me understand why "master" was replaced with "tip"? I tried reading the PR description and commit messages, and didn't find a rationale there.

The latest Travis CI documentation says:

or use master to get the latest version from source

That's why this change is unexpected for me and I'd like to understand it better.

@dmitshur

dmitshur Sep 8, 2018

Member

@marwan-at-work Can you please help me understand why "master" was replaced with "tip"? I tried reading the PR description and commit messages, and didn't find a rationale there.

The latest Travis CI documentation says:

or use master to get the latest version from source

That's why this change is unexpected for me and I'd like to understand it better.

This comment has been minimized.

@marwan-at-work

marwan-at-work Sep 10, 2018

Contributor

@dmitshur im not sure but it failed for me and switched to tip because we use tip at athens. I don't know the difference, but maybe we can open a pr and check?

@marwan-at-work

marwan-at-work Sep 10, 2018

Contributor

@dmitshur im not sure but it failed for me and switched to tip because we use tip at athens. I don't know the difference, but maybe we can open a pr and check?

This comment has been minimized.

@gmlewis

gmlewis Sep 11, 2018

Collaborator

I can do a quick experiment.

Update: https://travis-ci.org/google/go-github/builds/426983699 LGTM.

@gmlewis

gmlewis Sep 11, 2018

Collaborator

I can do a quick experiment.

Update: https://travis-ci.org/google/go-github/builds/426983699 LGTM.

This comment has been minimized.

@gmlewis

gmlewis Sep 11, 2018

Collaborator

See #1002 for successful run with master.

@gmlewis

gmlewis Sep 11, 2018

Collaborator

See #1002 for successful run with master.

If you're interested in using the [GraphQL API v4][], the recommended library is
[shurcooL/githubv4][].
## Usage ##
```go
import "github.com/google/go-github/github"
import "github.com/google/go-github/github/v18"
```

This comment has been minimized.

@dmitshur

dmitshur Sep 8, 2018

Member

Given that modules were released very recently and are still experimental/optional, perhaps it's a good idea to expand this to have two sections: one for traditional GOPATH workspaces (with import "github.com/google/go-github/github"), and another section for module-aware environments (with import "github.com/google/go-github/v18/github").

@dmitshur

dmitshur Sep 8, 2018

Member

Given that modules were released very recently and are still experimental/optional, perhaps it's a good idea to expand this to have two sections: one for traditional GOPATH workspaces (with import "github.com/google/go-github/github"), and another section for module-aware environments (with import "github.com/google/go-github/v18/github").

This comment has been minimized.

@marwan-at-work

marwan-at-work Sep 10, 2018

Contributor

@dmitshur yes, and probably should include go get part too?

@marwan-at-work

marwan-at-work Sep 10, 2018

Contributor

@dmitshur yes, and probably should include go get part too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment