-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
Initial attempt at #42 #44
base: master
Are you sure you want to change the base?
Conversation
sync.py
Outdated
|
||
self.dir = os.path.join(clone_dir, name) | ||
self.check_call = functools.partial(subprocess.check_call, cwd=self.dir) | ||
|
||
def clone(self, init_submodules=True): | ||
if not os.path.isdir(os.path.join(self.clone_dir, self.name)): | ||
logging.info('Cloning meta repository %s', self.name) | ||
subprocess.check_call(shlex.split('git clone --depth 1 [email protected]:{}/{}.git'.format(ORGANIZATION, self.name)), cwd=self.clone_dir) | ||
subprocess.check_call(shlex.split('git clone --depth {} [email protected]:{}/{}.git'.format(self.sync_depth, ORGANIZATION, self.name)), cwd=self.clone_dir) | ||
if init_submodules and self._has_submodules(): | ||
self.check_call(shlex.split('git submodule update --depth 1 --init --jobs 8')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the same depth apply to this? Should we have a different parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same depth. There shouldn't be any need for a separate one.
ref. #42. Also, this is completely untested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from the minor nit. Should indeed be tested :)
sync.py
Outdated
self.clone_dir = clone_dir | ||
self.name = name | ||
self.submodules = submodules | ||
self.author = author | ||
self.sync_depth = sync_depth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind changing this to just depth? The sync prefix inside the sync class is a bit redundant.
I can test this on mock-apertium. What depths should I test with (i.e., what is the problem that this is supposed to solve)? |
I think the problem is when things get into an inconsistent state and |
@sushain97 Ping about testing this. |
Yeah I'm a bit anal-retentive so I deleted it right after the real one was
created. I suppose you can recreate?
…On Thu, Apr 12, 2018, 11:41 AM Shardul Chiplunkar ***@***.***> wrote:
@sushain97 <https://github.com/sushain97> Ping about testing this.
mock-apertium has been deleted?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEBEfnOjC8mprQif4Uz6fy7Z5RXmKq_Kks5tn4PTgaJpZM4TBKc->
.
|
No description provided.