[Output/Entry] Changes entries `output` method to `pretty_value` property

[ingrinder]

Description

I mentioned this already in #146 - this approach is a "bit more OOP". I find it easier to reason about output getting the property directly from each entry than a callback (output.output) calling a callback (entry.output) calling a callback (output.append)!!

Reason and / or context

Partly cleaner, partly because it makes it easier to cache these properties for #146 :grin:

How has this been tested ?

Seems to work alright on my system, unit tests updated accordingly

Types of changes :

• Bug fix (non-breaking change which fixes an issue)
• [X] Typo / style fix (non-breaking change which improves readability) -- I think? Unless someone is directly trying to access Entry.output I don't think there's any end-user difference
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

• [IF NEEDED] I have updated the README.md file accordingly ;
• [X] [IF NEEDED] I have updated the test cases (which pass) accordingly ;
• [?] [IF BREAKING] This pull request targets next Archey version branch ;
• [X] My changes looks good ;
• [X] I agree that my code may be modified in the future ;
• [X] My code follows the code style of this project (PEP8).

[ingrinder]

Apologies for the mess of commits -- my Pylint VS Code integration was silently broken, I didn't have mypy installed, and I'm not totally familiar with type hinting in Python yet!

[ingrinder]

I really need to fix Pylint :wink:

Another thought: After (or even before...) this change, some of the pretty_value (or equivalent output) overrides could be instead replaced by defining __str__ for the entry. In particular:

• Distro
• Kernel
• Load Average
• RAM
• Temperature
• Terminal
• Uptime
• Window Manager

As these all essentially just change the way their value is formatted to a string. This just leaves the following entries with a custom pretty_value(/output) for multi-line output:

• CPU
• Custom
• Disk
• GPU
• LAN_IP
• WAN_IP

In fact, looking at the value each of these entries holds, it may be possible to incorporate multi-line output into the normal Entry.pretty_value, and narrow this list down to only Disk. Let me know what you think.

[HorlogeSkynet]

Hello Michael !

That's good work again :pray: You're right about Output.output messiness. Let's DRY (again) the code base.

These changes are somewhat breaking regarding Entry API, but I guess we can go for it anyway (chances that someone actually use them externally are very low ?).

Some notes :

• If you import typing in new sheets, I'd advise we put import typing at the top instead of the from typing import ... form in order to simplify future diffs (i.e. we won't need to add/remove types to/from types list) ;
• You can DRY List[tuple[str, str]] type by creating your own (under entry module for instance) with typing.Type ;
• I haven't properly reviewed #146 yet, but I guess some (persistent) cache (e.g. under .cache/) for entries could be a simpler/quicker way to deal with "slow" entries ;
• Do you think pretty_value property actually returns "pretty values" ? :sweat_smile: From my POV it rather pre-formats entry for output, but maybe I'm bike-shedding here ;
• Getting rid of callbacks simplifies unit tests, and that's awesome !

Bye, have a nice WE :wave:

[ingrinder]

Hi!

I haven't properly reviewed https://github.com/HorlogeSkynet/archey4/pull/146 yet

I would hold off for now, it's very much a mess :wink:

As for the custom type, I didn't know about that! As you say, it's cleaner -- I'll change it soon.

Do you think pretty_value property actually returns "pretty values" ? 😅 From my POV it rather pre-formats entry for output, but maybe I'm bike-shedding here ;

I thought something similar, but it's hard to come up with a name. We already have value for the canonical data of the entry; output_value or output seemed a little confusing to me. Have you got any ideas for what it could be called?

[HorlogeSkynet]

I thought something similar, but it's hard to come up with a name. We already have value for the canonical data of the entry; output_value or output seemed a little confusing to me. Have you got any ideas for what it could be called?

As we return somehow a list, maybe values (it may be misleading too :sweat_smile:). Or we simply could override the __iter__ magic method which could yield list members :wink:

[ingrinder]

@HorlogeSkynet I was thinking about this for a while and I think you were right in overriding the magic methods to make an iterable. What do you think of this approach? In general a lot of entries have lines removed (simpler? :thinking:), and the output handling is shifted more into Output (particularly the "Not detected" strings, since we should probably only be dealing with them in one place!!)

To-do, still:

• New tests for Output now functionality has changed.
• Re-factoring of Output.output? 7 levels of indentation scares me :laughing:

It might be a while before I get around to those, I've got exams next month!

See you!

[HorlogeSkynet]

Impressive work @ingrinder :innocent:

In general a lot of entries have lines removed (simpler? 🤔), and the output handling is shifted more into Output (particularly the "Not detected" strings, since we should probably only be dealing with them in one place!!)

:+1:

It might be a while before I get around to those, I've got exams next month!

Ignore this notification and spend more time on this when you'll want/be able to ! I planned a new release for around ~August this year. Nonetheless, good luck for your exams :muscle:

---

First review :

• CPU : if I read it well, I guess your refactoring breaks support for multiple cores (.items() may return multiple values, one per CPU core, for each socket).
• Disk (and others) : instead of defining Self, you can use "Disk" (note the quotes) for static typing (for "older" Python).
• Disk too : I'm not sure about "placeholder", is it a leftover ?
• Kernel (and others, including in tests !) : "str" -> str
• your ValueType new type can be defined in entry.py module, instead of being defined as an Entry class attribute
• I am not sure about Entry.__iter__ isinstance(self.value, (str, int)) test ; Other base types could land here. Maybe we can iter(tuple(self.value)) from the moment self.value is not iterable instead ?
• Do you think we can combine the raise StopIteration of Entry.__next__ ?

---

Bye ! :wave:

[ingrinder]

Ignore this notification and spend more time on this when you'll want/be able to ! I planned a new release for around ~August this year.

I've got time spare to go over your comment :)

CPU : if I read it well, I guess your refactoring breaks support for multiple cores (.items() may return multiple values, one per CPU core, for each socket).

Since it grabs from _iter_value it does iterate over the list (from self.value). As far as I can see, each of the dict structures in the list is only used to have a "name" associated with the count, and all the parsing methods we have only ever have one entry (appending to the list multiple times for multiple CPUs).

Maybe this is not the right structure, and say a list of tuples would be more appropriate? (it would be a breaking change though, particularly for JSON output)

Disk (and others) : instead of defining Self, you can use "Disk" (note the quotes) for static typing (for "older" Python).

Ah, thanks! Definitely simpler that way.

Disk too : I'm not sure about "placeholder", is it a leftover ?

You're right, this is a stub from while I was rewriting the other entries. I had a # todo: comment to catch it in linting which I guess I accidentally removed!! :eyes:

Kernel (and others, including in tests !) : "str" -> str your ValueType new type can be defined in entry.py module, instead of being defined as an Entry class attribute

Thanks for catching, will sort these next time I work on it :smile:

I am not sure about Entry.__iter__ isinstance(self.value, (str, int)) test ; Other base types could land here. Maybe we can iter(tuple(self.value)) from the moment self.value is not iterable instead ?

The problem with that approach is that strings are iterable but we don't really want to iterate over them when the entry value consists of only a string, otherwise we will get an entry line per character! It made most sense to me at the time to set up an iterator where the sequence returned matches the lines of the entry.

Other than that, I agree that it's probably a better solution to try __iter__ on the value first -- but I feel like it ended up being more messy once having to deal with some of the other cases.

Do you think we can combine the raise StopIteration of Entry.__next__ ?

Yes, I don't see why not, e.g. changing the first test to if self._iter_idx > 0 or not self.value and removing the second test entirely.

Thanks for taking a look over it. I'll be back soon! :wave: