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:
- I didn't use types to make the script work with Python 2 and 3. Is it a good practice?
- Does the script follow Python style conventions? I didn't manage to find unified Python style guides, I used what was provided by
flake8
. - Do you usually cover scripts like this with Unit tests? How would you do that?
- 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: [email protected]:user/repo.git or
https://github.com/user/repo.git""")
return parser.parse_args()
if __name__ == "__main__":
main()
-
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– AlexV2019年08月29日 15:35:54 +00:00Commented Aug 29, 2019 at 15:35
-
2\$\begingroup\$ @AlexV added the full script \$\endgroup\$Sasha Shpota– Sasha Shpota2019年08月30日 20:36:24 +00:00Commented Aug 30, 2019 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– Reinderien2019年09月02日 14:43:57 +00:00Commented Sep 2, 2019 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\$Ben A– Ben A2019年09月04日 04:25:20 +00:00Commented Sep 4, 2019 at 4:25
1 Answer 1
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
...
-
3\$\begingroup\$ And if you're not concerned about Python 2 compatibility, you could also use
pathlib
, which is really quite nice. \$\endgroup\$grooveplex– grooveplex2019年08月29日 17:23:17 +00:00Commented Aug 29, 2019 at 17:23 -
2\$\begingroup\$ @grooveplex Added a short part on that. \$\endgroup\$Graipher– Graipher2019年08月29日 17:29:36 +00:00Commented Aug 29, 2019 at 17:29
-
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– Sasha Shpota2019年08月30日 20:45:26 +00:00Commented Aug 30, 2019 at 20:45