Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added bits of refactoring, and a test, removed python27 in tox #48

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Added bits of refactoring, and a test, removed python27 in tox #48

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4066a81
refactor(runner) added bits of refactoring, and a test, removed pytho…
Jun 2, 2022
cc02977
add strict assert to test
Jul 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dirsync/run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import os

from .syncer import Syncer
from .options import ArgParser, USER_CFG_FILE, DEFAULT_USER_CFG

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason the options module was only imported within from_cmdline is that it has side effects (creating the options dictionary + arg parser class, etc.), which is not needed at all when calling dirsync from python. That's why I would prefer only importing it from within from_cmdline. If the tests are still needed, they could import ArgParser from the options module.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personal I do not like import in functions. I like to have all the import at the top of the file where I would expect them to find. When I look at the options module, I think having the import of the options module only for cmd_line scope, would be a premature optimization.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like imports in functions either and endeavour to put all imports at the top, but in a few cases like this one they make sense. Imports in python are pretty expensive (especially as the options module imports other modules) so if it is easy to avoid importing modules we know we won't need in some well-defined use cases, we do it.

A change that is trivial to implement and understand and can save 10s of ms each time a module is imported is not 'premature optimisation' (a term that has been debased), it's just common sense!

If we really wanted to avoid imports in functions, you could have create a separate from_cmdline module that would import the bits it needs from options, but as from_cmdline and run essentially do the same thing, it makes a lot of sense to keep them in the same module.

Would you mind reverting that change please? Thanks in advance.


def sync(sourcedir, targetdir, action, **options):
Expand All @@ -22,7 +23,6 @@ def sync(sourcedir, targetdir, action, **options):


def from_cmdline():
from .options import ArgParser, USER_CFG_FILE, DEFAULT_USER_CFG

# create config file if it does not exist
user_cfg_file = os.path.expanduser(USER_CFG_FILE)
Expand Down
96 changes: 49 additions & 47 deletions dirsync/syncer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@
from .options import OPTIONS
from .version import __pkg_name__

PERMISSION_777 = 1911
PERMISSION_666 = 1638

class DCMP(object):
"""Dummy object for directory comparison data storage"""

def __init__(self, l, r, c):
self.left_only = l
self.right_only = r
Expand Down Expand Up @@ -247,78 +250,77 @@ def _dowork(self, dir1, dir2, copyfunc=None, updatefunc=None):
continue

# Files & directories only in source directory
for f1 in self._dcmp.left_only:
try:
st = os.stat(os.path.join(self._dir1, f1))
except os.error:
continue
for file, st in self.iter_files(self._dir1, self._dcmp.left_only):
if stat.S_ISREG(st.st_mode) and copyfunc:
copyfunc(file, self._dir1, self._dir2)
self._added.append(os.path.join(self._dir2, file))

if stat.S_ISREG(st.st_mode):
if copyfunc:
copyfunc(f1, self._dir1, self._dir2)
self._added.append(os.path.join(self._dir2, f1))
elif stat.S_ISDIR(st.st_mode):
to_make = os.path.join(self._dir2, f1)
to_make = os.path.join(self._dir2, file)
if not os.path.exists(to_make):
os.makedirs(to_make)
self._numnewdirs += 1
self._added.append(to_make)

self.update_common_files(updatefunc)

def update_common_files(self, updatefunc):
# common files/directories
for f1 in self._dcmp.common:
for file, st in self.iter_files(self._dir1, self._dcmp.common):
if stat.S_ISREG(st.st_mode) and updatefunc:
updatefunc(file, self._dir1, self._dir2)

@staticmethod
def iter_files(base_path, files):
"""Yield file and it's statistics, ignore when errors"""
for file in files:
try:
st = os.stat(os.path.join(self._dir1, f1))
yield file, os.stat(os.path.join(base_path, file))
except os.error:
continue

if stat.S_ISREG(st.st_mode):
if updatefunc:
updatefunc(f1, self._dir1, self._dir2)
# nothing to do if we have a directory

def _copy(self, filename, dir1, dir2):
def _copy(self, filename, source_dir, target_dir):
""" Private function for copying a file """

# NOTE: dir1 is source & dir2 is target
if self._copyfiles:

rel_path = filename.replace('\\', '/').split('/')
rel_dir = '/'.join(rel_path[:-1])
filename = rel_path[-1]

dir2_root = dir2
target_root = target_dir

dir1 = os.path.join(dir1, rel_dir)
dir2 = os.path.join(dir2, rel_dir)
source_dir = os.path.join(source_dir, rel_dir)
target_dir = os.path.join(target_dir, rel_dir)

if self._verbose:
self.log('Copying file %s from %s to %s' %
(filename, dir1, dir2))
(filename, source_dir, target_dir))
try:
# source to target
if self._copydirection == 0 or self._copydirection == 2:

if not os.path.exists(dir2):
if not os.path.exists(target_dir):
if self._forcecopy:
# 1911 = 0o777
os.chmod(os.path.dirname(dir2_root), 1911)
os.chmod(os.path.dirname(target_root), PERMISSION_777)

try:
os.makedirs(dir2)
os.makedirs(target_dir)
self._numnewdirs += 1
except OSError as e:
self.log(str(e))
self._numdirsfld += 1

if self._forcecopy:
os.chmod(dir2, 1911) # 1911 = 0o777
os.chmod(target_dir, PERMISSION_777)

sourcefile = os.path.join(dir1, filename)
sourcefile = os.path.join(source_dir, filename)
try:
if os.path.islink(sourcefile):
os.symlink(os.readlink(sourcefile),
os.path.join(dir2, filename))
os.path.join(target_dir, filename))
else:
shutil.copy2(sourcefile, dir2)
shutil.copy2(sourcefile, target_dir)
self._numfiles += 1
except (IOError, OSError) as e:
self.log(str(e))
Expand All @@ -327,28 +329,27 @@ def _copy(self, filename, dir1, dir2):
if self._copydirection == 1 or self._copydirection == 2:
# target to source

if not os.path.exists(dir1):
if not os.path.exists(source_dir):
if self._forcecopy:
# 1911 = 0o777
os.chmod(os.path.dirname(self.dir1_root), 1911)
os.chmod(os.path.dirname(self.dir1_root), PERMISSION_777)

try:
os.makedirs(dir1)
os.makedirs(source_dir)
self._numnewdirs += 1
except OSError as e:
self.log(str(e))
self._numdirsfld += 1

targetfile = os.path.abspath(os.path.join(dir1, filename))
targetfile = os.path.abspath(os.path.join(source_dir, filename))
if self._forcecopy:
os.chmod(dir1, 1911) # 1911 = 0o777
os.chmod(source_dir, PERMISSION_777)

sourcefile = os.path.join(dir2, filename)
sourcefile = os.path.join(target_dir, filename)

try:
if os.path.islink(sourcefile):
os.symlink(os.readlink(sourcefile),
os.path.join(dir1, filename))
os.path.join(source_dir, filename))
else:
shutil.copy2(sourcefile, targetfile)
self._numfiles += 1
Expand All @@ -371,15 +372,15 @@ def _cmptimestamps(self, filest1, filest2):
else:
return mtime_cmp

def _update(self, filename, dir1, dir2):
def _update(self, filename, source_dir, target_dir):
""" Private function for updating a file based on
last time stamp of modification or difference of content"""

# NOTE: dir1 is source & dir2 is target
if self._updatefiles:

file1 = os.path.join(dir1, filename)
file2 = os.path.join(dir2, filename)
file1 = os.path.join(source_dir, filename)
file2 = os.path.join(target_dir, filename)

try:
st1 = os.stat(file1)
Expand All @@ -398,14 +399,15 @@ def _update(self, filename, dir1, dir2):
# source file's modification time, or creation time. Sometimes
# it so happens that a file's creation time is newer than it's
# modification time! (Seen this on windows)
need_upd = (not filecmp.cmp(file1, file2, False)) if self._use_content else self._cmptimestamps(st1, st2)
need_upd = (not filecmp.cmp(file1, file2, False)) if self._use_content else self._cmptimestamps(st1,
st2)
if need_upd:
if self._verbose:
# source to target
self.log('Updating file %s' % file2)
try:
if self._forcecopy:
os.chmod(file2, 1638) # 1638 = 0o666
os.chmod(file2, PERMISSION_666)

try:
if os.path.islink(file1):
Expand All @@ -418,9 +420,9 @@ def _update(self, filename, dir1, dir2):
shutil.copy2(file1, file2)
self._changed.append(file2)
if self._use_content:
self._numcontupdates += 1
self._numcontupdates += 1
else:
self._numtimeupdates += 1
self._numtimeupdates += 1
return 0
except (IOError, OSError) as e:
self.log(str(e))
Expand All @@ -445,7 +447,7 @@ def _update(self, filename, dir1, dir2):
self.log('Updating file %s' % file1)
try:
if self._forcecopy:
os.chmod(file1, 1638) # 1638 = 0o666
os.chmod(file1, PERMISSION_666)

try:
if os.path.islink(file2):
Expand Down
24 changes: 24 additions & 0 deletions tests/test_run_from_cmdline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from unittest.mock import patch
from dirsync import run


def test_run_from_cmd_line_then_create_user_config_file_when_not_exist():
with patch.object(run, "os") as os_mock:
os_mock.path.isfile.return_value = False

with patch("builtins.open") as mock_file:
with patch.object(run, "sync"), patch.object(run, "ArgParser"):
run.from_cmdline()

assert mock_file.called
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't we want to check a few more things? i.e. with what arguments open() has been called?



def test_run_from_cmd_line_error_with_exit_2():
with patch.object(run, "os") as os_mock:
os_mock.path.isfile.return_value = True

with patch.object(run, "sync"), patch.object(run, "ArgParser") as arg_parser_mock:
arg_parser_mock.side_effect = Exception()
with patch.object(run, "sys") as sys_mock:
run.from_cmdline()
sys_mock.exit.assert_called_with(2)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that test checking that the script exits when the arg parser raises an exception? If yes I'm not sure I understand the point of running it ... but maybe I'm missing something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I see this part of the test coverage, without this test the exception context of the try sync is not tested. My aim is always to have 100% coverage on my code, as experience has shown that the bugs usually hide in the few percent not covered.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I made that remark believing from_cmdline was already tested (see below) and that there would be quite a lot of more relevant not-covered parts of dirsync that would deserve attention first. But you definitely have a point, and the fact that from_cmdline was not tested at all before makes it a lot more relevant.

I indeed realised that the cmdline test module where your tests could (and probably should) be located only called dirsync.run.run. I think the CmdLineTests.dirsync method should be updated there to call from_cmdline instead of dirsync.run, and the arguments should be fed into sys.args instead of being passed.

In this module you'll see that the decorator syntax (@patch(...)) is preferred to the context manager syntax (with patch(...)) for all patches required in the test case. This gets rid of quite a few indents and makes the test a lot more readable.

If you are happy with that, it would be great to:

  • modify tests.cmline.CmdLineTests.dirsync so that it calls from_cmdline
  • move your tests in the tests.cmdline module and make them inherit from CmdLineTests
  • use @patch on the test method to avoid the context managers in your tests

Sorry for not having had a closer look in my previous review and thanks again for the contributions.

2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[tox]
envlist = py27, py36, py37, py38, py39
envlist = py36, py37, py38, py39
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree 2.7 should be dropped (there is a bit of clean-up to do to fully remove six and python 2 specific code). I'll drop 3.6 too, and add 3.10 (and soon 3.11)


[testenv]
deps =
Expand Down