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

Adding a Default Action in 12-cmdline.md solution fails on piped stdin #843

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

rshpeley
Copy link
Contributor

@rshpeley rshpeley commented Jul 3, 2020

Solution readings09.py fails on piped stdin and no action argument:

python ../code/readings_09.py < small-01.csv
Traceback (most recent call last):
  File "../code/readings_09.py", line 33, in <module>
    main()
  File "../code/readings_09.py", line 7, in main
    action = sys.argv[1]
IndexError: list index out of range

This error occurs because there is no argument for action or filename. The fix adds a conditional test to handle the case where no args exist.

The readings_09.py file in the code folder would also need to be updated for this fix.

Instructions

Thanks for contributing! ❤️

If this contribution is for instructor training, please email the link to this contribution to
checkout@carpentries.org so we can record your progress. You've completed your contribution
step for instructor checkout by submitting this contribution!

Keep in mind that lesson maintainers are volunteers and it may take them some time to
respond to your contribution. Although not all contributions can be incorporated into the lesson
materials, we appreciate your time and effort to improve the curriculum. If you have any questions
about the lesson maintenance process or would like to volunteer your time as a contribution
reviewer, please contact The Carpentries Team at team@carpentries.org.

You may delete these instructions from your comment.

- The Carpentries

Solution readings09.py fails on piped stdin and no action argument:
``` bash
python ../code/readings_09.py < small-01.csv
Traceback (most recent call last):
  File "../code/readings_09.py", line 33, in <module>
    main()
  File "../code/readings_09.py", line 7, in main
    action = sys.argv[1]
IndexError: list index out of range
```
This error occurs because there is no argument for action or filename. The fix adds a conditional test to handle the case where no args exist. 

The `readings_09.py` file in the `code` folder would also need to be updated for this fix.
Copy link
Contributor

@ldko ldko left a comment

Hi @rshpeley ,
Thank you for pointing out and submitting a fix for the case that the script is called with no default action and is using stdin. Your fix works for me. I have a couple small comments only on your changes, but before merging this, you would also need to update the code in code/readings_09.py, which reflects the code in the solution. Thanks again!

> > filenames = sys.argv[2:]
> >
> > action = sys.argv[1] # it's safe now to get action argument
> > if action not in ['--min', '--mean', '--max']: # if other action given
Copy link
Contributor

@ldko ldko Jul 14, 2020

Choose a reason for hiding this comment

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

Here the comment was changed from "If no action given" to "if other action given" which seems a minor tweak, but in the case of another action (something made up or a typo) given vs. no action given, moving up the index to get filenames will error on an OSError when the other action is treated as a filename. I don't think we necessarily need to remedy this possible error too for the sake of the exercise, however, I think it would be good for you to change the comment back to "if no action given".

> > if action not in ['--min', '--mean', '--max']: # if no action given
> > action = '--mean' # set a default action, that being mean
> > filenames = sys.argv[1:] # start the filenames one place earlier in the argv list
> > if len(sys.argv) == 1: # there are no args but there may be a pipe
Copy link
Contributor

@ldko ldko Jul 14, 2020

Choose a reason for hiding this comment

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

Small style suggestion: changing to use two spaces before # and sticking to that throughout the code would help it look more consistent (and adhere to PEP-8). (I realize the spaces were inconsistent before you worked on this :).)

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