wilas / vbkick

Tool for building and maintaining VirtualBox VMs described as a code in a single definition file.
Other
8 stars 5 forks source link

This should implement the key features of Issues #37 and #38. #39

Open cniswander opened 10 years ago

cniswander commented 10 years ago

It doesn't give vbkick the ability to interpret millisecond expressions like <123>, but I think that is relatively low priority? Also, I think Wilas could edit vbkick much better than I could.

wilas commented 10 years ago

Great work @cniswander :) Thanks for a pull request :)

I did a code review - most things is about following python code guide.

If you don't agree with me or you think that something could/should be done better (or if I'm wrong in some points) feel free to comment here.

1) docstring instead of comments for all modules, functions, classes, and methods. This apply as well to the current convert_2_scancode functions. - Low prior and could be fixed later.

2) comments before line of code not inside. Base on PEP8: http://legacy.python.org/dev/peps/pep-0008/#comments - "Block comments generally apply to some (or all) code that follows them, and are indented to the same level as that code."

convert_2_scancode is not PEP8 compatible yet but the goal is to make it.

3) number of spaces - must be 4, not mix 2 and 4

Example from PR:

+    for k, v in get_naked_multi_char_codes().items():
+      scancodes['<%s>' % k] = v

probably some vim settings should be added to bottom of the file to avoid similar issues in future:

#  As per: http://vimdoc.sourceforge.net/htmldoc/options.html#'tabstop'
# Set 'tabstop' and 'shiftwidth' to whatever you prefer and use 'expandtab'.
# vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4

4) get_metakey_codes() should be similar to get_naked_multi_char_codes() - so I think dictionary should be created explicit: keycodes['Shift'] = ('2a', 'aa') # I think left shift.

5) It a good practice to not change a type of variables in python, e.g. keys_array is sometimes a list and sometimes a boolean

Fix: def translate_sleeps(input, keys_array=False) -> def translate_sleeps(input, keys_array=[])

if keys_array == False: -> if not keys_array:

There is more like this e.g.: def translate_meta(input, keys_array=False)

6) Long string concatenations + regexpr - make it a bit more readable.

What about something like this:

def named_meta_group(group_name):
    metakey_re = '|'.join(['(%s)' % x for x in metakey_code_keys]) # extra ) brackets from orig PR are not needed
    return '(?P<%s>%s)' % (group_name, metakey_re)

Similar approach can be used for: pattern = ( r'\<' ...

7) be more py3 safe - http://blog.labix.org/2008/06/27/watch-out-for-listdictkeys-in-python-3 - low prior and there will be more refactoring needed in future. Usually we are safe as dict is not modify during the iteration. Example:

From:

metakey_code_keys = list(metakey_codes.keys())
metakey_code_keys.sort(reverse=True)

To:

metakey_code_keys = sorted(metakey_codes.keys(), reverse=True)

8) if metakey_codes.get(x, False): -> if x in metakey_codes: - because you never use a default get value, but the if.

9) if 0: pass... - this should use the DEBUG constant (otherwise these bits are not needed in the code). In near future this could be replaced by _log("string") function or other logger with various logging levels.

10) Catch exception - because translate_meta(input, keys_array) raise Exception, it should be catch later in translate_chars(input, support_millisecond_expressions=False) to exit with code 1 and print nice message to stderr.

11) definition of "quasi private" functions should start with single underscore (base on http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html#naming):

I think following function we can call a "private" (in big short - all helping methods):

def create_meta_regex(): -> def _create_meta_regex() 
def name_of_ith_meta_group(i): -> def _name_of_ith_meta_group(i)
def de_duplicate(mylist, omit_false=True): -> def _de_duplicate(mylist, omit_false=True):

Public are all get, translate and process_multiply.

12) simplify def de_duplicate(mylist, omit_false=True): What about:

def _de_duplicate(mylist, omit_false=True):
    uniq_elms = set(mylist)
    if omit_false:
        return list(filter(None, uniq_elms)) 
    return list(uniq_elms)

NB: As an alternative but I didn't make a performance tests: return [x for x in uniq_elms if x]

13) Other bits can be refactored later. e.g we can use defaultdict and deque: https://docs.python.org/2/library/collections.html#collections.deque to replace construtions like post.insert(0, release)

wilas commented 10 years ago

I think I've written to much... and I wanted too much at once. "Move fast" should be crucial and 2 "small" steps for dealing with patches. Apply patch if implements required features. Do refactoring to increase a project quality. Not everything at once. I failed with this in my previous comment.

PR created by the @cniswander implements needed features and I'm happy to add these changes. Code refactoring can be done later. The only one thing worth changing now is a "number of indent" - everywhere in python code 4.

Thanks @cniswander for your work and contribution :) waiting for your another PR fixing "indents" so that I can merge changes.