(fix) ilab diff for knowledge doc bug #888
Conversation
5717652 to
edb33aa
Compare
|
@bjhargrave @aartij22 fyi |
Closes #885 Fix: `working_dir` arg being passed to `os.path.join()` for `glob.glob` was of type `Repo`. Fix is to pass in `Repo.working_dir`, which is of type `str` instead. Also fixes an issue with `skip_checkout` variable, which was always `False`. Fix is to use a temp variable `sc` and set `sc` to `True/False` based on `skip_checkout` Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
edb33aa to
d3bcafa
Compare
| @@ -206,22 +206,23 @@ def get_documents( | |||
| file_patterns = source.get("patterns") | |||
| with tempfile.TemporaryDirectory() as temp_dir: | |||
| try: | |||
| skip_checkout = False | |||
| sc = False | |||
There was a problem hiding this comment.
The sc variable is not necessary. Just get rid of the three lines and pass the skip_checkout argument to git_clone_checkout.
There was a problem hiding this comment.
git_clone_checkout expects a bool and here skip_checkout can be None. So we would need
if skip_checkout is None:
skip_checkout = Falseto ensure it has a bool value.
There was a problem hiding this comment.
Then you should change the type annotations to require a bool and not allow None.
There was a problem hiding this comment.
Optional[T] does not mean optional argument. It is syntactic sugar for Union[T, None].
| working_dir = temp_dir | ||
| # that needs to be processed instead of the cloned project inside | ||
| # temp_dir | ||
| working_dir = repo |
There was a problem hiding this comment.
working_dir = repo looks wrong and is very confusing. It only works because the tests are violating type annotation and return str instead of a Repo instance from a mock-patched git_clone_checkout() function.
|
Please take a look at #893. It solves the problem differently with a proper spec-ed Mock object. |
|
Closing in favor of #893 |
Closes #885
Fix:
working_dirarg being passed toos.path.join()forglob.globwas of typeRepo. Fix is to pass inRepo.working_dir, which is of typestrinstead.Also fixes an issue with
skip_checkoutvariable, which was alwaysFalse. Fix is to use a temp variablescand setsctoTrue/Falsebased onskip_checkoutChanges
Which issue is resolved by this Pull Request:
Resolves #
Description of your changes: