Portability Fixes #8
Conversation
rathod-sahaab
commented
Oct 18, 2018
rathod-sahaab
commented
Added support different package manager based on their availability, rather than based on distro in use. >Also> Added print_clr() to avoid ambiguities on non-error printed as errors, rather than print_err() which writes on 'stderr', for colour.
There was a problem hiding this comment.
Well done @mr-rathod,
I really appreciate the approach of package-managers.
I have made some comments on design changes.
please have a look on them
coolkit/dependencies/dependency.py
Outdated
| for rules in dependency_map.values(): | ||
| for key in rules.keys(): | ||
| package_managers.add(key) | ||
|
|
There was a problem hiding this comment.
keep it like your are returning supported package managers. not any one of them.
There was a problem hiding this comment.
and make changes accrodingly in install_dependencies
-Standardisation of names -Support for multiple package mangers -got rid of extra whitespaces
There was a problem hiding this comment.
good work @mr-rathod ,
I have made few reviews. please address them as soon as posssible.
I am looking forward to make a new release as soon as possible.
coolkit/dependencies/dependencies.py
Outdated
| 'dnf':'sudo dnf install python-argcomplete',#fedora | ||
| 'zypper':'sudo zypper install python-argcomplete',#suse | ||
| }, | ||
| 'figlet':{ |
There was a problem hiding this comment.
hei remove figlet. that was just for testing purpose.
coolkit/dependencies/dependency.py
Outdated
|
|
||
| supported_package_managers = set() | ||
| for package_manager in package_managers : | ||
| if is_installed(package_manager) : |
There was a problem hiding this comment.
@mr-rathod please dont check for installed ones in this method.
as per name of this method _get_supported_package_managers it has to return list of supported ones not available ones.
There was a problem hiding this comment.
you will have to create a new method def _install_dependency(supported_package_managers):
coolkit/dependencies/dependency.py
Outdated
| all_installed = True | ||
| package_managers = _get_supported_package_managers(dependency_map) |
There was a problem hiding this comment.
change this variable name to supported_package_managers
There was a problem hiding this comment.
you may get available_package_managers list in from another method which will take supported_package_managers as input.
coolkit/dependencies/dependency.py
Outdated
| package_managers = _get_supported_package_managers(dependency_map) | ||
|
|
||
| #--handling-absence-of-package-managers-- | ||
| if len(package_managers)==0 : |
There was a problem hiding this comment.
if len(available_package_managers) == 0:
coolkit/dependencies/dependency.py
Outdated
| print_clr('.:Installing ' + C.E + dependency + C.G +' dependency' , C.G) #distinction of dependency | ||
|
|
||
| for package_manger in package_managers : | ||
| os.system(rules[package_manager]) |
There was a problem hiding this comment.
only run rules of available_package_managers
rule_map = dependency_map[dependency]
for package_manager in available_package_manager:
if(package_manager in rule_map):
os.system(......)
.....
coolkit/dependencies/dependency.py
Outdated
|
|
||
| if not is_installed(dependency): | ||
| print_err('please install ' + dependency + ' dependency manually',C.Y) | ||
| for package_manager in package_managers : |
There was a problem hiding this comment.
for package_manager in available_package_managers:
Available package managers are selected from supported package managers for each dependency in dependency map
There was a problem hiding this comment.
Now the PR looks good to me, I have requested some changes. most of them are minor code polishing changes. Most of the changes I suggested are github suggestions so you can apply them from online just click on apply change. after you update the PR I will test it and merge it :)
Co-Authored-By: Mr-Rathod <abhayonlyone@gmail.com>
Co-Authored-By: Mr-Rathod <abhayonlyone@gmail.com>
Co-Authored-By: Mr-Rathod <abhayonlyone@gmail.com>
actions are now handled together rather than prompts
|
Well done @mr-rathod , I am merging this PR :) |
