7
\$\begingroup\$

I implemented a Python script that generates git commits for the last year making one's Contribution Graph look full of changes. It was my first Python program (I work mostly with Java). Although the program is quite useless I want to use it as a learning playground to build my Python competences. That is why I would appreciate any feedback about the code (both constructive and destructive).

Questions:

  1. I didn't use types to make the script work with Python 2 and 3. Is it a good practice?
  2. Does the script follow Python style conventions? I didn't manage to find unified Python style guides, I used what was provided by flake8.
  3. Do you usually cover scripts like this with Unit tests? How would you do that?
  4. And generally what would you improve? Any comments are appreciated.

Code: (applied the changes proposed by Graipher)

You can find the full script here.

def main():
    args = arguments()
    curr_date = datetime.now()
    directory = 'repository-' + curr_date.strftime('%Y-%m-%d-%H-%M-%S')
    repository = args.repository
    if repository is not None:
        start = repository.rfind('/') + 1
        end = repository.rfind('.')
        directory = repository[start:end]
    no_weekends = args.no_weekends
    frequency = args.frequency
    os.mkdir(directory)
    os.chdir(directory)
    run(['git', 'init'])
    start_date = curr_date.replace(hour=20, minute=0) - timedelta(366)
    for day in (start_date + timedelta(n) for n in range(366)):
        if (not no_weekends or day.weekday() < 5) \
                and randint(0, 100) < frequency:
            for commit_time in (day + timedelta(minutes=m)
                                for m in range(contributions_per_day(args))):
                contribute(commit_time)
    if repository is not None:
        run(['git', 'remote', 'add', 'origin', repository])
        run(['git', 'push', '-u', 'origin', 'master'])
    print('\nRepository generation ' +
        '\x1b[6;30;42mcompleted successfully\x1b[0m!')

def contribute(date):
    with open(os.path.join(os.getcwd(), 'README.md'), 'a') as file:
        file.write(message(date) + '\n\n')
    run(['git', 'add', '.'])
    run(['git', 'commit', '-m', '"%s"' % message(date),
        '--date', date.strftime('"%Y-%m-%d %H:%M:%S"')])

def run(commands):
    Popen(commands).wait()

def message(date):
    return date.strftime('Contribution: %Y-%m-%d %H:%M')

def contributions_per_day(args):
    max_c = args.max_commits
    if max_c > 20:
        max_c = 20
    if max_c < 1:
        max_c = 1
    return randint(1, max_c)

def arguments():
    parser = argparse.ArgumentParser()
    parser.add_argument('-nw', '--no_weekends',
                        required=False, action='store_true', default=False,
                        help="""do not commit on weekends""")
    parser.add_argument('-mc', '--max_commits', type=int, default=10,
                        required=False, help="""Defines the maximum amount of
                        commits a day the script can make. Accepts a number
                        from 1 to 20. If N is specified the script commits
                        from 1 to N times a day. The exact number of commits
                        is defined randomly for each day. The default value
                        is 10.""")
    parser.add_argument('-fr', '--frequency', type=int, default=80,
                        required=False, help="""Percentage of days when the
                        script performs commits. If N is specified, the script
                        will commit N%% of days in a year. The default value
                        is 80.""")
    parser.add_argument('-r', '--repository', type=str, required=False,
                        help="""A link on an empty non-initialized remote git
                        repository. If specified, the script pushes the changes
                        to the repository. The link is accepted in SSH or HTTPS
                        format. For example: git@github.com:user/repo.git or
                        https://github.com/user/repo.git""")
    return parser.parse_args()

if __name__ == "__main__":
    main()
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! This site has a very generous limit on the amount of code you can put in a question. Since you are ought to have all the code that you want to have reviewed in the question and your script is not super massive, it's likely best to show it in full length. \$\endgroup\$ – AlexV Aug 29 '19 at 15:35
  • 2
    \$\begingroup\$ @AlexV added the full script \$\endgroup\$ – Sasha Shpota Aug 30 '19 at 20:36
  • \$\begingroup\$ You say that you support Python 2 because it's your OS default, but... that's not great. If you write a shebang with python3, your script will use the right thing and your code can be improved. \$\endgroup\$ – Reinderien Sep 2 '19 at 14:43
  • \$\begingroup\$ To anyone looking at the edit history: I rolled it back after seeing that the OP changed the code, then realizing that they added the full script at the request of @AlexV, I rolled the post back to the updated code. \$\endgroup\$ – Linny Sep 4 '19 at 4:25
7
+50
\$\begingroup\$

Python is actually well-known for having a unified style-guide. It is called PEP8. It has both general advice as well as specific advice (naming and such). flake8 follows the recommendations of PEP8, AFAIK.

Currently your script only runs on UNIX system due to the way you manually handle file paths for the directory variable. If you were consistently using the os.path module instead, it would run also on Windows.

import os

if args.repository is not None:
    directory = os.path.splitext(os.path.basename(args.repository))[0]
else:
    directory = 'repository-' + curr_date.strftime('%Y-%m-%d-%H-%M-%S')

I would add a os.chdir(directory) somewhere in the beginning. This way you don't need to pass the directory to every call of perform. I would leave it as an argument (in case you do need it), but make it optional. I would also call the function run.

def run(commands, context_dir=None):
    if context_dir is None:
        context_dir = os.getcwd()
    Popen(commands, cwd=context_dir).wait()

Actually, that is already the default behaviour of Popen!. So you can just do:

def run(commands, context_dir=None):
    Popen(commands, cwd=context_dir).wait()

In your argument parsing a few things are superfluous. While "explicit is better than implicit", "simple is better than complex". All arguments that start with a - or -- are automatically optional, so you don't need the required=False. In addition, with action="store_true", default=False is implied.

type=str is also redundant, since that is the default for all arguments.

The repository option should maybe have a nargs=1 keyword to ensure an actual argument is passed.

I would pass an optional argument to the parsing, which let's you test the function with a list of strings:

def arguments(args=None):
    ....
    return parser.parse_args(args)

print(arguments(["-r"]))

If you were not concerned about Python 2 compatibility (it's time is finally coming to an end, slowly), I would have recommended using the new high-level API subprocess.run (Python 3.5+, 3.7+ for some features). With that your function would not even be needed anymore, a simple from subprocess import run would be enough.

In that vain, you could then also use pathlib, as recommended in the comments by @grooveplex:

from pathlib import Path

if args.repository is not None:
    directory = Path(args.repository).stem
...
\$\endgroup\$
  • 3
    \$\begingroup\$ And if you're not concerned about Python 2 compatibility, you could also use pathlib, which is really quite nice. \$\endgroup\$ – grooveplex Aug 29 '19 at 17:23
  • 2
    \$\begingroup\$ @grooveplex Added a short part on that. \$\endgroup\$ – Graipher Aug 29 '19 at 17:29
  • \$\begingroup\$ Thank you for the review (+1). \$\endgroup\$ – Sasha Shpota Aug 30 '19 at 7:19
  • 1
    \$\begingroup\$ @Graipher I applied most of your recommendations and added the full script to the question. I kept Python 2 compatibility simply because my latest Ubuntu LTS still defaults to python 2. As for Windows compatibility, I was not clear enough - the args.repository is a git repository link (either HTTPS or SSH format). It should also work on Windows (though, I had no chance to test it). \$\endgroup\$ – Sasha Shpota Aug 30 '19 at 20:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.