-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
doc/code.html: update code.html to use go modules #28218
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
This PR (HEAD: aedc7dd) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
Message from Gerrit User 5976: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
This PR (HEAD: a04f722) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 72b2c69) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
doc/code.html
Outdated
@@ -202,25 +202,35 @@ <h3 id="ImportPaths">Import paths</h3> | |||
</p> | |||
|
|||
<p> | |||
We'll use <code>github.com/user</code> as our base path. Create a directory | |||
inside your workspace in which to keep source code: | |||
We'll use <code>github.com/user</code> as our base path for our Go programs. Create a new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should stop using github.com/user as the base path for our program with modules. Instead, they should just be able to do something like create a folder on their desktop or in Documents or something. Also this looks like a windows-specific path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, as stated on the wiki entry about modules, the opt-in nature of modules in go 1.11 currently requires that modules be used outside of a GOPATH
unless the GO111MODULE
environment variable is correctly set (GO111MODULE=on
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdwhatcott I wasn't sure whether to write this from a Go 1.12 point of view, in which setting the environment variable will be redundant (I think?). I'll update this though to reflect current release and ensure it's easily delete-able.
@deanveloper If the go mod init
command can't automatically determine the module path from the likes of VCS meta-data then you need to supply the module path. I felt keeping github.com/user was consistent with the rest of the content already there and would minimize confusion if the user hadn't already run git init
+ git remote add origin ...
.
To both - I can add a line that explicitly states that the new project directory be created outside of the GOPATH
and within a directory the user is comfortable with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good @elliotforbes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdwhatcott + @deanveloper I've updated the page to include both your suggestions. Have a look and let me know what you think :)
This PR (HEAD: 03c2e5f) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 0567c22) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
doc/code.html
Outdated
@@ -302,7 +315,7 @@ <h3 id="Command">Your first program</h3> | |||
</p> | |||
|
|||
<pre> | |||
$ <b>cd $GOPATH/src/github.com/user/hello</b> | |||
$ <b>cd ~/myproject/hello</b> | |||
$ <b>git init</b> | |||
Initialized empty Git repository in /home/user/work/src/github.com/user/hello/.git/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the output of this git init
command needs to be updated:
Initialized empty Git repository in /home/user/work/src/github.com/user/hello/.git/ | |
Initialized empty Git repository in /home/user/myproject/hello/.git/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I've updated it!
This PR (HEAD: 01ca903) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
This PR (HEAD: bcdb292) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
oops ill fix the cla stuff, apparently I get attributed if I suggest a change? |
Signed it although looks like @elliotforbes needs to comment to get it to check :P |
I signed it! |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
doc/code.html
Outdated
$ <b>go install</b> | ||
$ <b>go install github.com/user/hello</b> | ||
$ <b>$HOME/go/bin/hello</b> | ||
hello, world. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we run the code further down in the tutorial, don't need it twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, you may have to comment "I signed it!" though, looks like the original attempt may have failed.
Won't let me comment inline for this one, as no changes were made to this area, but here we still have GOPATH code in the libraries section |
Looks like we'll need a member to set the CLA label |
doc/code.html
Outdated
</p> | ||
|
||
<pre> | ||
$ <b>mkdir $GOPATH/src/github.com/user/stringutil</b> | ||
$ <b>mkdir ~/myproject/hello/stringutil</b> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's better it's a separate package rather than a subpackage of github.com/user/hello
?
Also need to change some code a few lines up so this "suggestion" only really applies to the line this comment was made on 😅
Also don't forget go mod init github.com/user/stringutil
!
$ <b>mkdir ~/myproject/hello/stringutil</b> | |
$ <b>mkdir ~/myproject/stringutil</b> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that originally, but AFAIK it requires a VCS repository behind it? You'd have to ask them to create a repo, create the package and then go through all the steps in order to import the package to their original application which seems slightly complex for what should be an introductory article?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, and it would be pretty annoying to do... Not sure in this case. Maybe it would be good to have it as a subdirectory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cover remote packages further down in the document, might be worthwhile extending that section to show an example of this in more depth and keeping the first bit simpler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've deleted other comments related to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cover remote packages further down in the document, might be worthwhile extending that section to show an example of this in more depth and keeping the first bit simpler?
Are there other documents explaining importing external packages? If so then we shouldn't need to do this
hello.go # command source | ||
stringutil/ | ||
reverse.go # package source | ||
</pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing all of this? It's useful information. It'll need the go.mod
files in there though
Message from thepudds: Patch Set 23: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Andrew Bonventre: Patch Set 23: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
This PR (HEAD: a697395) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
Message from Elliot Forbes: Patch Set 24: (1 comment)
I've updated the list here taking your comments into account, let me know if these need further work. Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Elliot Forbes: Patch Set 24:
This should hopefully be fixed now, it was an auto-format issue. Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Andrew Bonventre: Patch Set 24: Code-Review+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Agniva De Sarker: Patch Set 24: (4 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
This PR (HEAD: 84bc96b) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 28bbe51) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
Message from Elliot Forbes: Patch Set 26: (4 comments)
I believe I've addressed all comments, let me know if anything further is needed :) Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Agniva De Sarker: Patch Set 26:
Thanks Elliot. Since you have marked this CL as "Updates #28215", I believe you have more content planned ? It would be good if you can create a CL series updating the page in chunks. Then it can be reviewed keeping the final state of the page in mind. Also adding Rob to take a look. Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
This PR (HEAD: 012ead1) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/142297 to see it. Tip: You can toggle comments from me using the |
Message from Agniva De Sarker: Patch Set 27: Elliot, would you mind updating this with the latest and resolve the merge conflicts ? Then we can take this forward. Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Andrew Bonventre: Patch Set 27: adding Jay to determine direction on this. Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
Message from Jay Conrod: Patch Set 27:
Not quite ready to review this yet. It's likely we'll need to re-outline and rewrite this page and move it to x/website. I'm planning to do that in the next couple weeks as part of golang.org/issue/33637. Sorry for the delay here. Please don’t reply on this GitHub thread. Visit golang.org/cl/142297. |
53bd915
to
6139019
Compare
4a7ed1f
to
0f992b9
Compare
This PR is being closed because golang.org/cl/142297 has been abandoned. Ended up doing a larger rewrite in CL 199417. |
This starts to update the doc/code.html article to use go modules.
Updates #28215