pyNES icon indicating copy to clipboard operation
pyNES copied to clipboard

better deal from command line when file is not found

Open gutomaia opened this issue 10 years ago • 8 comments

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.

gutomaia avatar Apr 27 '14 03:04 gutomaia

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.

danilobellini avatar Apr 27 '14 04:04 danilobellini

@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.

gutomaia avatar Apr 27 '14 04:04 gutomaia

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

popmilo avatar Apr 27 '14 11:04 popmilo

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 ?

popmilo avatar Apr 29 '14 09:04 popmilo

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

danilobellini avatar Apr 29 '14 09:04 danilobellini

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 avatar Apr 29 '14 10:04 popmilo

@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 :)

popmilo avatar Apr 29 '14 11:04 popmilo

@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!

gutomaia avatar Apr 30 '14 01:04 gutomaia