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

Inline the code that produces errors #272

Merged
merged 2 commits into from
Feb 3, 2017

Conversation

Ry4an
Copy link
Contributor

@Ry4an Ry4an commented Jun 13, 2016

An instructor commented that it's useful to have the students type/paste the code into their console so they can see the error themselves. In the error_02 case seeing the code isn't necessary to understand the error message or answer the six questions, but finding the actual bug is a useful follow up question.

@VardaHagh
Copy link

+1

2. The second shows some code in another function (`favorite_ice_cream`, located in the file `errors_01.py`),
with an arrow pointing to Line 7 (which is `print(ice_creams[3])`).
2. The second shows some code in another function (`favorite_ice_cream`,
located in the file `errors_01.py`, shown in its entirty below) with an
Copy link
Contributor

Choose a reason for hiding this comment

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

entirity -> entirety

Copy link
Contributor Author

Choose a reason for hiding this comment

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

entirity -> entirety

Good catch. On it.

@tbekolay
Copy link
Contributor

I'm a bit conflicted about this PR... if you want to show the contents of the file, you could cat code/errors_01.py during a lesson. I do see the benefit for people reading through the lesson offline, but the focus here is on looking at the error in isolation, which is the situation programmers most commonly find themselves in. Including the text in multiple places makes it possible / likely that the contents of the file will differ from what we have in the topic.

What do you think about removing the file entirely and just having it within the lesson? To me, it feels a bit distracting... but I think that we should either keep the file or the text in the topic, not both.

@Ry4an
Copy link
Contributor Author

Ry4an commented Jun 13, 2016

I see where you're coming from. I do think whomever created the original issue is right that there's probably an "ah ha" available to a learner who not only learns to parse the traceback but then also identifies the bug in the code that caused it. It doesn't seem to me that's likely to happen without the code being right there in the lesson, but that's okay too. I'm guessing the error python files were only retained in code/ only in case the error message needed to be regenerated in the future. They're not findable and probably not worth linking to in github.

@tbekolay
Copy link
Contributor

Could we get some more opinions on this? Make a comment, or emoji respond to this comment:

  • ❤️ for including the code in the topic and removing code/errors_01.py and code/errors_02.py from the repository
  • 😆 for keeping the files and adding a note to do cat code/errors_01.py to see the code
  • 👎 for keeping things as they are

@jesford
Copy link

jesford commented Jun 16, 2016

I ❤️ the idea of putting the error-generating code inline.

The main issue with the current lesson is that learners (and instructors) are never told to download those code/errors_*.py files. The lesson just starts with import errors_01 which will not work unless there are instructions for obtaining that code. Same issue for viewing the file with cat.

The time it takes to make sure everyone downloads those files and has them in their path is greater than the time it takes to just type those two short examples into a notebook cell.

@valentina-s
Copy link
Contributor

My reason for supporting the inline code inclusion is that importing of a file is not taught up to that point (only importing of general libraries) so this could add an extra level of confusion for the students. In the lesson we can stress that understanding of the code is not important for answering the questions.

@gvwilson gvwilson self-assigned this Jun 22, 2016
@valentina-s
Copy link
Contributor

@tbekolay @Ry4an @gvwilson

I am looking into resolving this PR which we did not finish last summer. I could go ahead and resolve the conflicts to merge it, and then also remove the reference to the files, thus leaving only the inline code. My only concern is that the code to type is a bit a lot. Should we try to shorten it, or maybe stress in the instructor notes that students do not have to retype it, only if they want to.

@Ry4an
Copy link
Contributor Author

Ry4an commented Jan 9, 2017

@valentina-s lemme take a pass at it tonight with the goal of (1) fixing the merge errors, (2) removing the external files, and (3) shortening it just a bit. Then you can merge it if it suits. There were 2 votes for keeping the error-producing code separate, but folks speaking up seem to mostly support inclusion. Ultimately your call, of course.

@tbekolay
Copy link
Contributor

tbekolay commented Jan 9, 2017

but folks speaking up seem to mostly support inclusion

I voted for keeping them separate, but the majority voted for inclusion so I'm good to go ahead with it once the issues are dealt with 👍

@Ry4an Ry4an closed this Jan 11, 2017
@Ry4an Ry4an force-pushed the issue-51-source-links branch from 3ec6eb9 to 4f56886 Compare January 11, 2017 05:07
@Ry4an
Copy link
Contributor Author

Ry4an commented Jan 11, 2017

Made current with master, inlined the two files with a message suggesting the learner may wish to use the code as a reference rather than typing it, removed the two code files. Also updated line numbers and etc. to keep the error output current.

@Ry4an Ry4an reopened this Jan 11, 2017
@valentina-s
Copy link
Contributor

Thanks, @Ry4an. As a compromise, I think we can remove the explicit functions for the callout Reading Error Messages. This is the longest one, and since the goal here is really reading it can be treated as an exercise which is shown to the participants (from the webpage, not typed in the notebook). What do you think?

@Ry4an
Copy link
Contributor Author

Ry4an commented Jan 31, 2017

I think we can remove the explicit functions for the callout Reading Error Messages. This is the longest one, and since the goal here is really reading it can be treated as an exercise which is shown to the participants (from the webpage, not typed in the notebook). What do you think?

I think I see what you're saying -- you don't want the learners encouraged to type out a long buggy function, and I completely agree. I do think that they have the best chance of reading and understanding the stacktrace if the code is present and line-numbered and that's most easily done by leaving it as a python code block.

What if I change the instructions on the exercise to:

Read the python code and the resulting traceback below and answer the following questions

and change the comment at the start of the code to:

# This code has an intentional error. Do not type it directly;
# use it for reference to understand the error message below.

I made those changes to the PR. Does that look okay, or are we really worried about the scroll length of the page when that exercise is expanded?

@valentina-s
Copy link
Contributor

It looks good to me now. I think the instructors can simply display the exercise from the browser and focus on the error output. If needed they can look at the actual code during the discussion of the solution.

@valentina-s
Copy link
Contributor

Thanks a lot!

@valentina-s valentina-s merged commit 49638bb into swcarpentry:gh-pages Feb 3, 2017
maxim-belkin pushed a commit that referenced this pull request May 23, 2018
Remove root and layout from YAML for boilerplate
zkamvar pushed a commit that referenced this pull request Apr 21, 2023
Inline the code that produces errors
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.

6 participants