Pylama gjslint - lukaszpiotr/pylama_with_gjslint GitHub Wiki

impression about the task:

  1. I do understand the purpose of the tool as to check various aspects of PYTHON style and PYTHON code quality. Personally I think it is a bad idea to pollute the tool for PYTHON with JS aspects. Either the name of the tool should inform that this is not just for Python (Pylama suggest pure python) or dedicated for JS wrapper should be done as separate solution. Of course this is just my subjective feeling which I wanted to shard with you - requested task is done as defined by requirements.

  2. Tool impression. At the time I was solving the task I had a feeling that nobody tested Pylama on Win platforms. The support for Win platform is poor (yes I know Linux rocks etc. but numbers are clear (06/04/2013): Windows 94,57 %, Linux 1,36 %, Other...% ). Packages which are required to run (correctly) tests are not mentioned in the specification. Author of the tool did not predict that line endings can change between Unix and Win systems, and tests results can be different depending on a platform.

  3. I did not check all of the aspects of licenses which are being used between wrapped solutions. However I am aware that there are some differences between BSD (Pylama), Apache License, Version 2.0 (gjslint) and MIT (other), which at the time of real deployment should be taken into consideration.

  4. Printing utilities were removed from gjslint.py module due to common printing interface defined by Pylama. Other flags are left in the code for further development, not all of them are "active code" right now but in my opinion it does not make any sense to remove functional (not printing) part of the library.

  5. Documentation of added code is limited, however in order to follow given in Pylama code convention I did not extend the doc strings.

  6. Added implementation is covered with tests.

impression about the Pylama code

  • In my opinion, usage of select does not make any sense :
 def filter_errors(e, select=None, ignore=None, **params):
    """ Filter a erros by select and ignore options.

    :return bool:

    """
    if select:
        for s in select:
            if e['text'].startswith(s):
                return True

    if ignore:
        for s in ignore:
            if e['text'].startswith(s):
                return False

    return True

It actually does not matter what did you define in select if you did not use ignore to limit the output, I guess expected behavior is to treat ignore/select as blacklist/whitelist - not as it is implemented right now.

  • bugs (perhaps):
  • Running on Win from src code on one partition and pointing to "code to be tested" on the other partition results in error.
  • If curses is not installed on Win, test are failing.
  • Some test are failing due to trailing whitespaces, which are handled differently on Unix and Windows.
  • In my opinion sorting - sorted x['lnum'] - is also invalid because Errors appear on the screen in following order:
    888 Issue ...
    88  Issue ...
    9999 Issue ...