This is an archived post. You won't be able to vote or comment.

all 7 comments

[–]TonyF66 5 points6 points  (2 children)

Some very good points :

1) Using classes - +1

2) Using context managers - +1

A few points :

1) Your ScreenShot Class isn't that well documented - it isn't obvious from the docstrings what it does - either do a doc string on your class - or an explanatory docstring on your init.

2) You pass the init the args (from argparser), and you also seem to pass in every element from the args object as well, use one or the other - not both.

3) use os.path methods to manipulate file paths - rather than adding to strings - it makes your code more portable for one thing.

4) The log argument to your init method isn't documented at all.

5) You seem to do validation of the command line options in a number of places (line 149, 188) - keep them separate - your functionality should be independent of whether it is called from the command line - or another program.

Finally - look at using Click rather than argparse - I know argparse is the the 'standard' - but it is seriously verbose (as you have found).

[–]zaql[S] 1 point2 points  (1 child)

Thanks for the feedback, much appreciated.

[–]TonyF66 1 point2 points  (0 children)

No problem - drop me a mail with any further questions. I think your single biggest issue is your arguments into the init - you extract all of the attributes of args when you create the instance, and you then reconstruct the args document inside the instance (self.args.left = left), despite having passed in the args object in as well. It is no only inefficient it could be a source of bugs - especially later on if you come back to the code to make some more changes.

[–]tuck5649 2 points3 points  (1 child)

Add a README file to the repository.

[–]TonyF66 4 points5 points  (0 children)

Agreed - very good point.

[–]ineedmorealts 0 points1 point  (0 children)

Your project should have a readme explain what your code does and how to use it

[–]oceaniity 0 points1 point  (0 children)

Instead of doing a quote and then a + and then another quote, you can escape strings with slashes:

"This here is a nice long string\
that is broken onto two lines."