gutomaia / pyNES

Python programming for Nintendo 8 bits
BSD 3-Clause "New" or "Revised" License
1.21k stars 109 forks source link

better deal from command line when file is not found #13

Open gutomaia opened 10 years ago

gutomaia commented 10 years ago

At the command line when calls: "pynes asm example.asm -o example.nes", you receive an ugly stack trace on the file doesn't exists. User must receive a better error handle in that case.

danilobellini commented 10 years ago

This should be quite simple, calling parser.error after a os.path.isfile verification (which also returns False when the file doesn't exist, so there's no need for calling os.path.exists explicitly). It would be somewhat similar to what was done here.

You can also use the type=argparse.FileType("r"), but that already opens the file and I don't know how it ensures that the file is closed afterwards, if that's really enforced by it.

gutomaia commented 10 years ago

@danilobellini We can give a try. I mean, I found two errors due to a file not found.

@popmilo what do you think about this task? @danilobellini could also help you and discuss possible alternatives, however he is already engaged in another task.

popmilo commented 10 years ago

Yeah, I can do that. I'll start working on it.

popmilo commented 10 years ago

Finally managed to spend some more time reading main code, really well written and interesting. Here is just a quick fix for this issue. Is it ok in this way (check if input file is valid and continue if it is) ?

   args = parser.parse_args(argv[1:])
   if is_valid_file(parser, args.input):
        args.func(args)

def is_valid_file(parser, arg):
    if os.path.isfile(arg):
        # File exists
        return True
    else:
        parser.error('The file {} does not exist!'.format(arg))
        return False

This is the result of "pynes asm example.asm -o example.nes": "usage: pynes [-h] {py,chr,asm,nt,img} ... pynes: error: The file example.asm does not exist!"

Should there be a check for output file name validity also ?

danilobellini commented 10 years ago

This can be slightly simpler, since the parser.error already terminates the program.

popmilo commented 10 years ago

You are right :) This piece of code inserted before "args.func(args)" does the job:

    if not os.path.isfile(args.input):
        parser.error('The file {} does not exist!'.format(args.input))
popmilo commented 10 years ago

@gutomaia Should I make a pull request for just this one small commit, or work on something else ? ps. I never thought I'll read and learn this much about Git when I found out about pyNes :)

gutomaia commented 10 years ago

@popmilo I've just take a look on that, it look's nice. However I, to help improve the community and the software also, the best approach to solve a request is to write a test.

What you have done so far is great. Problem is, if someone like me, break your code. No one will notice 'cause there is tests related to that part.

On the project there is a commandline_test.py, first take a look on it. Try to figure out what test would break the application. If you need any help, just ask! I'm just trying to lead you into a path, not just give you the answer!

Good luck!