Closed Tobilike closed 8 months ago
I made a mistake with Uptime. I have improved it. Don't really know how to change it here. Is it automatic?
Hi again @Tobilike!
I'll give this a full review later on. For the time being I have a few thoughts after a skim:
output
and decide there whether to print them or not. At the least we need to move the check into e.g. __init__
rather than using class attributes (which may cause issues with multiple entries of the same type, particularly Custom
).icons.py
file?Let me know what you think :wave:
Hi @ingrinder
* Would it be possible to rebase this onto our master, particularly so that the changes from [Added the endeavouros logo #140](https://github.com/HorlogeSkynet/archey4/pull/140) and your master branch aren't part of this PR?
Sure, I have no problem with that. GitHub is still new to me and I have to figure out how it all works. If I do something differently than I should, please give me a hand so I have a better understanding. Can also remove my fork again, just don't quite know how else to submit something.
* On adding icons: instead of making a configuration check and prepending the icon to the name in each entry, I would suggest it may be better to e.g. pass an icon to `output` and decide there whether to print them or not. At the least we need to move the check into e.g. `__init__` rather than using class attributes (which may cause issues with multiple entries of the same type, particularly `Custom`).
I'll see what I can do. I wasn't aware that this could cause problems. It is certainly smarter to call it once instead of calling it x number of times.
* What's the purpose of the 9k+ `icons.py` file?
I wrote everything the file does into the file. I have not used it yet, but the possibility exists. I found all the icons I used. It is also best to visit the website https://www.nerdfonts.com/cheat-sheet .
Usage:
import icons
print (icons.icons) # for a list of all icons
print (icons.icons['fa_thumbs_up']) # for printing one icon with the name fa_thumbs_up
output:
{'cod_account': '\ueb99', 'cod_activate_breakpoints': '\uea97', ..., ..., ...,
'weather_wind_west': '\ue354', 'weather_windy': '\ue31e'}
👍
Greetings
Now everything should be fine. I have reworked it again. Also tested tested on a other System. (Ubuntu)
Nice, this is definitely looking like a cleaner solution! :+1:
There are a couple of minor things left as far as I can see:
Output
and print a message about changing the unicode option, do we need to mention this option also (will using the icons throw unicode errors?)# icon
/# icon and name
comments should go, as _ICON
and _PRETTY_NAME
seems pretty self-explanatory.Pylint
, black
and isort
over the code, I noticed there is an unused Configuration
import in kernel.py
, unusued os
import in window_manager.py
and some changed spacing in lan_ip.py
.Rebasing might be a bit tricky if you're not used to git yet, but here's the approach I'd recommend if you think you're up to it to solve the conflicts we have now (take a backup of the folder just in case...!):
upstream
: git remote add upstream https://github.com/HorlogeSkynet/archey4.git
git fetch upstream
upstream-master
: git checkout -b upstream-master upstream/master
git checkout Icons
git rebase --interactive upstream-master
, which gives you a text editor interface to cherry-pick commits.pick
the relevant commits and drop
the others. For example, I would drop
a16a91b
through 38a9c28
, the two commits d62c6d4
and 0e244b0
(as these belong in your other PR), and 6691a8a
(as it deletes a file in a dropped earlier commit and will fail). I think the rest should be fine as they are as they should squash down fairly cleanly.git rebase --continue
after saving and closing this file as some of your commits were empty.git push --force
If you can't manage I'm sure we (by which I mean @HorlogeSkynet, as I can't modify commits before merging PRs :grin:) can clean it up when merging for you.
Ciao! :wave:
A picture says more
I have added a config option to switch the icons on and off as above. However, a nerd font is required for this to work, otherwise you will only see placeholders. https://github.com/ryanoasis/nerd-fonts "icon": true,
Reason and / or context
I like it.
How has this been tested ?
With my start.py
Types of changes :
Checklist :
[IF BREAKING] This pull request targets next Archey version branch ;