search

Macchina-CLI / libmacchina

A library providing access to all sorts of system information.
https://crates.io/crates/libmacchina
MIT License
68 stars 20 forks source link

Explicitly mark readouts that are not implemented #102

Closed FantasyTeddy closed 2 years ago

FantasyTeddy commented 2 years ago

This is probably a controversial change. The idea behind this is to make it more obvious, which readouts/functions are supported in the corresponding targets.

Advantage: It is easier to determine which functions are implemented for a specific target and what might need to be added in the future.

Disadvantage: It might seem messier to have all those ReadoutError::NotImplemented.

❗ Important: I did not yet make the change in the entire library and it therefore does not compile yet. I want to gather some feedback if this is even something that would be accepted, before I finish this work.

grtcdr commented 2 years ago

How this is any different from:

static ref STANDARD_NO_IMPL: ReadoutError = ReadoutError::Warning(String::from(
    "This metric is not available on this platform or is not yet implemented by macchina."
));

The PR is titled "explicity mark readouts that are not implemented", but by default, any unimplemented functions are going to result in ""This metric is not available on this platform or is not yet implemented by macchina.", which achieves the same goal as what you're trying to do, but with more lines of code. For an end user (developer), there is no noticeable change.

FantasyTeddy commented 2 years ago

The difference is that my suggestion is 'explicit' while the current implementation is 'implicit'. In the current implementation you have to know which functions any specific readout offers and compare it with the functions that you have actually implemented. You don't see immediately, what functionality is not implemented.

For me these are two different answers to the following question: "When do I want to realize that I forgot to implement a function?"

grtcdr commented 2 years ago

You don't see immediately, what functionality is not implemented.

Your point is solid, let's see what the others think.

@uttarayan21 @123marvin123

uttarayan21 commented 2 years ago

I think this is a good change, I always try to be explicit with errors and seems like this might be useful when debugging readouts.

grtcdr commented 2 years ago

In the current implementation you have to know which functions any specific readout offers and compare it with the functions that you have actually implemented.

Yeah, resulting in a lot of wasted time.

FantasyTeddy commented 2 years ago

Great, I will continue working on this, since your feedback seems to be mostly positive.

I have one more question that would make it easier for me: could we make the GitHub Workflow of this repository more like the one in macchina where we test every possible target? This would give me (or any other contributor) more confidence that I didn't screw something up while refactoring.

grtcdr commented 2 years ago

could we make the GitHub Workflow of this repository more like the one in macchina where we test every possible target?

Sure, I'll make this happen.

123marvin123 commented 2 years ago

You don't see immediately, what functionality is not implemented.

Your point is solid, let's see what the others think.

@uttarayan21 @123marvin123

I think its a good change 👍🏼

grtcdr commented 2 years ago

If you've noticed, a lot of our documentation is written for implementors, but we've yet to see anybody implement on top of libmacchina, we should write documentation that explains how to use the functions and their expected output.

This is a topic for another day, though.

FantasyTeddy commented 2 years ago

I noticed that too. For this pull-request I will implement the missing functions in the documentation examples, but I agree that the examples should rather target the users than those that implement readout traits.

grtcdr commented 2 years ago

Something's up with the crates.io API :(

FantasyTeddy commented 2 years ago

Yes, apparently they are experiencing an outage.

grtcdr commented 2 years ago

We should refrain from pushing until they can recover, we don't want to be stressing their servers at this moment.

FantasyTeddy commented 2 years ago

According to their incident report, the issue should be resolved. Can you trigger the CI pipeline again?

grtcdr commented 2 years ago

According to their incident report, the issue should be resolved. Can you trigger the CI pipeline again?

Right away ;)

grtcdr commented 2 years ago

We're set! You've done an outstanding job @FantasyTeddy :heart:

FantasyTeddy commented 2 years ago

Thank you, glad I can help :)