Added support for installing applications from root - #3186
Conversation
|
I acknowledge the work you've done it adding this feature! 👍 I can't comment on the actual implementation itself, but I have some general suggestions to help improve this pull request. I think the commit messages need some work. Personally I like to start all my commits for a PR to either reference the PR# or the issue#, in this case #3186. This allows for helpful linking of the commit to the relevant discussion. I also don't think it is helpful to have 6 different commits which all have the exact same commit message. As it stands there are 6 commits and they all do various things. As they are separated the only way for me to know what each one does is to dig down into the change itself. If they had more accurate messages then I could tell at a glance what each commit was about, for example the last commit was translations for certain languages. An in-depth exposition which at a skimmed glance looks great for more reading is Github's Writing Better Commits. The other issue I saw was that on some of the translation files the spacing was changed from 4 spaces to 2. Whatever your personal preferences are it is best to conform to a project's internal conventions so that only the pertinent changes are transmitted (and not all of them). Regardless if the Obtanium maintainers accept your PR, I think implementing the above suggestions will help you be a better developer and contributor. Thank you for the work and good luck. |
Thanks for your comments, I will try to do this. |
|
The translations changes are much cleaner now in your PR. I've made a PR #3197 which would upgrade the english translations tab spaces from 2 to 4 (which should me merged prior to this one) to align with the rest of the translation files. |
Thanks, but I already did that. |
Yes, I saw! :) It's a difference of opinion in scope. |
|
Successful working after the merge |
- Add RootInstaller class for installing apps using root permissions - Root installer checks for su availability in PATH
No description provided.