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.

Sign up for GitHub

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

Jump to bottom

doc/articles/wiki: move _ warning slightly higher at one point #22617

Closed
sewi-cpan opened this issue Nov 7, 2017 · 5 comments
Closed

doc/articles/wiki: move _ warning slightly higher at one point #22617

sewi-cpan opened this issue Nov 7, 2017 · 5 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@sewi-cpan
Copy link

Currently working myself through https://golang.org/doc/articles/wiki/ and got stuck at the first HTTP server example. It doesn't have any error handling but easily runs into a bad Go panic

2017/11/07 20:08:24 http: panic serving 127.0.0.1:48718: runtime error: invalid memory address or nil pointer dereference

Took me some time to find out that I was missing the test.txt file. Created a solution comment before noticing that pull requests to are not allowed but don't want to go through the full contributor process right now just to add few lines to the tutorial page.

You may copy from my commit or write something else, but there should be a hint for others running into the same problem:
sewi-cpan@c47c017
@ALTree ALTree changed the title Beginners trap in tutorial: hhtp panic invalid memory address or nil pointer dereference doc: beginners trap in tutorial: hhtp panic invalid memory address or nil pointer dereference Nov 7, 2017
@odeke-em odeke-em changed the title doc: beginners trap in tutorial: hhtp panic invalid memory address or nil pointer dereference doc: beginners trap in tutorial: http panic invalid memory address or nil pointer dereference Nov 7, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Nov 7, 2017
@gbbr
Copy link
Member

gbbr commented Nov 9, 2017 •
edited

In my opinion no change is needed here, since it is clearly mentioned in the text:

Again, note the use of _ to ignore the error return value from loadPage. This is done here for simplicity and generally considered bad practice. We will attend to this later.

There is even a special section in the very same wiki document dedicated specifically to this issue (Handling non-existent pages)

@gbbr gbbr added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 9, 2017
@agnivade
Copy link
Contributor

agnivade commented Nov 9, 2017 •
edited

Agree. May be we can change this text This is done here for simplicity and generally considered bad practice. to bold ? So that there are fewer chances of getting tripped.

@rsc
Copy link
Contributor

rsc commented Nov 13, 2017

In the text:

First, this function extracts the page title from r.URL.Path, the path component of the request URL. The Path is re-sliced with [len("/view/"):] to drop the leading "/view/" component of the request path. This is because the path will invariably begin with "/view/", which is not part of the page's title.

The function then loads the page data, formats the page with a string of simple HTML, and writes it to w, the http.ResponseWriter.

Again, note the use of _ to ignore the error return value from loadPage. This is done here for simplicity and generally considered bad practice. We will attend to this later.

Let's move the final ("Again...") paragraph above the first two.

@rsc rsc changed the title doc: beginners trap in tutorial: http panic invalid memory address or nil pointer dereference doc/articles/wiki: move _ warning slightly higher at one point Nov 13, 2017
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 13, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 13, 2017
@rsc rsc modified the milestones: Unplanned, Go1.10 Nov 13, 2017
@sewi-cpan
Copy link
Author

sewi-cpan commented Nov 13, 2017 •
edited

The hint is ok, but the result of the "_" could be a problem: The unintended reader easily runs into what looks like a completely unrelated error while trying the source until this point. The "_" is perfectly ok at this place and also the reference to attend to this later.
I read the "_" warning. The consequences might be obvious for Go experts but not for newbies walking through the tutorial.

Another idea to reduce the risk of problems here: The first example (in "Data Structures") does create a "TestPage.txt", but the webserver example discussed here expects "test.txt". Why not just use the same name in both examples. Doing so would create the test file needed for part 2 in part 1.

@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 4, 2017
@gopherbot
Copy link

Change https://golang.org/cl/87375 mentions this issue: doc/articles/wiki: highlight the use of _ warning

@golang golang locked and limited conversation to collaborators Feb 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
7 participants
@rsc @agnivade @sewi-cpan @ianlancetaylor @odeke-em @gbbr @gopherbot