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

WIP: Units plugin #2614

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@goJoWe16
Copy link
Contributor

commented Apr 10, 2019

Pull Request for Midterm submission of the Homework for CM 2019 SS. Work in Progress.

For now, only the correctness of given values + prefixes + SI-units is checked. Prefixes (such as k or µ) are not transformed until now. Furthermore, the units and prefixes are stored as they are (e.g. value + prefix + unit is stored). This is also intended to be changed in further steps, such that only the value is stored, the prefix is considered to transform the value to it's SI-Base unit and the SI-Unit is discarded.

Regarding issue #1398:
I am not sure if this is the intended behaviour of this plugin, as the correctness of SI-Base Units will be ignored in this case. For example, 1m would be correct when checking against the unit without a prefix (m interpreted as meter). In the example of issue #1398 , 1m could be interpreted as 1 milli.

@markus2330 could you give me some explanation of how you would expect to distinguish between the to possibilities in this case?
As far as i envisioned this plugin, a value with an optional prefix and a mandatory SI-unit has to be entered.

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

@markus2330 please review my pull request

@goJoWe16 goJoWe16 changed the title Units plugin WIP: Units plugin Apr 10, 2019

@kodebach

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Take a look at the example in #1398:

[key]
type = long
unit/base/#0 = decimal
unit/base/#1 = hexadecimal
unit/notation/#0 = metric
unit/notation/#1 = e
unit/measurement = s

The names aren't that important (but I think they work), but the basic idea is. The plugin would use the metakey unit/measurement to set which Unit to use. It also doesn't have to be SI, no reason to not allow fun units like the Barn-megaparsec (bMpc). So 1m is 1 meter if unit/measurement = m and 0.001 if unit/measurement is unset (or empty), otherwise it is an error.

I would also be nice to have some way of configuring how the values are converted. The default would be normalizing to the base unit (so km to m etc.). This might however require using floating point numbers, which could introduces errors. I think it would be good to have a second mode, where the plugin converts to the prefix given in e.g. unit/scale. This way you can control the precision of the floating point value. You could also add an option to always produce an integer and just ignore the decimal part, so that the type plugin can validate correctly.
(Note: it should be possible to convert the number without actually converting it from a string to an integer type, you just need to shift the decimal point)

For the whole procedure of normalizing to a number without a unit and restoring back to number + unit, you can look at the type plugin (especially the boolean and enum parts).

The other parts of #1398 (different bases, different notations) are nice to have, but its probably best, if you only implement the actual unit and SI-prefix parsing. You should however keep in mind, that we also want to implement the other stuff in future and write your code accordingly. So while using regex might be nice for the units and prefixes, I think it would get very complicated once the other stuff comes in (although it would probably work).

@markus2330
Copy link
Contributor

left a comment

Thank you, looks very nice!


## Introduction

This plugin provides the capability to enter values with additional SI units. The unit will be checked if it as a valid SI base-unit, additional SI prefixes will be validated and the value re-calculated to it's base-unit in respect to the prefix.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

it's -> its

## Examples

```sh
sudo kdb mount /tests/units.ini /tests/units ini units

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Is ini needed? Otherwise use the default (by not specifying "ini")

#> Check the /tests/units key for validity
kdb set /tests/units "1m"
#> Suceeds, since the value is a valid decimal with SI-unit representation.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

This needs to match with the output.

kdb set /tests/units "1m"
#> Suceeds, since the value is a valid decimal with SI-unit representation.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

What does kdb get now return? (also for other examples...)

#> Suceeds, since the value is a valid decimal with SI-unit representation.
kdb set /tests/units "32"
#> Throws an error: value of key is not a representation for a value with a SI-unit

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

Which error? (See shell recorder tests/shell/shell_recorder/tutorial_wrapper/README.md for how to specify errors)

description:value of key is not a valid SI-unit
severity:error
ingroup:plugin
module:units

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

add newline

@@ -235,6 +239,11 @@ The following section lists news about the [modules](https://www.libelektra.org/

- [cache](https://www.libelektra.org/plugins/cache) is a new global caching plugin. It uses [mmapstorage](https://www.libelektra.org/plugins/mmapstorage) as its storage backend and lazily stores keysets from previous ´kdbGet()´ calls. We added initial support for the default resolver and multifile resolver. _(Mihael Pranjić)_

### units

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

twice?

if (!meta) return 1;
const char* value = keyString(key);
const char * prefix = "(Y|Z|E|P|T|G|M|k|h|da|d|c|m|µ|n|p|f|a|z|y)";
const char * unit = "?(m|g|s|A|K|mol|cd|rad|sr|Hz|N|Pa|J|W|C|V|F|Ω|Ohm|S|Wb|T|H|C|lm|lx|Bq|Gy|Sv|kat|l|L)";

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

formatting broken

#include <string.h>
#include <stdlib.h>

int is_valid_key(Key * key, Key * parentKey)

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

can be static

regex_t regex;
regmatch_t offsets;
int compile_failure = regcomp (&regex, regexString, REG_NOSUB | REG_EXTENDED | REG_NEWLINE);
if (compile_failure) return -1;

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 14, 2019

Contributor

No error set when the compilation fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.