Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AsTypeDescription impl for human_repr types #170

Closed
wants to merge 2 commits into from

Conversation

matthiasbeyer
Copy link
Collaborator

Closes #169

@matthiasbeyer
Copy link
Collaborator Author

I somehow got things a bit wrong... the human_repr crate does not support parsing these,... which I thought it would.

I requested this functionality here: rsalmei/human-repr#4

But before this is implemented, we can postpone this PR I think.

@matthiasbeyer
Copy link
Collaborator Author

human_repr got the serde feature in 1.1, so this is now rebased and contains the crate in 1.1 with serde enabled.

@TheNeikos
Copy link
Owner

What happens if you put 1.0000028s with the human_repr crate? From what I read, it would not give you exactly that count, but instead round it, making it just 1. I would not consider this applicable for configuration use, where the precision of the user could very much matter.

I would accept the PR if it can be shown that:

  • Precision can optionally not be lost when parsing & printing
  • Tests that check that property, to make sure potential bugs don't get through

@matthiasbeyer
Copy link
Collaborator Author

Yeah now that I look at the code of the crate, they even implemented serde compatibility not as I intended. 😞

I think we can close this issue, as depending on the human_repr crate seems not to be a good idea.

@matthiasbeyer matthiasbeyer deleted the human-repr-impl branch April 19, 2023 12:02
@rsalmei
Copy link

rsalmei commented Apr 27, 2023

Hi @TheNeikos,
I'm the author of https://github.com/rsalmei/human-repr.
This use as "configuration" was never intended, human-repr was conceived to generate human representations for user messages, but I'm kinda linking this idea now...
I'm working on it, thinking about how I could make it parse things and allow one to write config files with these human representations, so this may be a reality soon.

  • Precision can optionally not be lost when parsing & printing

Of course in a configuration system precision is mandatory, rest assured I'm thinking about this.
What I'm worried about are units that might clash with prefixes.
See, on 123MCrabs it is obvious that we have 123 million of Crabs. And 123Crabs are just 123 of Crabs.
But what about 123Transactions? Is it 123 of Transactions or 123 billion of ransactions? The "C" is not a prefix, but "T" is the Tera one... I'm not sure how I'll resolve this (remove units from serialization altogether?).

Anyway, the issue @matthiasbeyer opened there on my crate is opened, so I expect this could be reopened too.
Thanks.

@TheNeikos
Copy link
Owner

Heya @rsalmei thanks for the notice. I appreciate the heads up, and I am definitely interested in integrating it with the changes you've suggested.

WRT to the units, you could maybe specify it as 123T Transactions or something, aka have a mandatory space between value+multiplier and unit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

human_repr implementation?
3 participants