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

Default to type NUMBER for variables #265

Merged
merged 4 commits into from
Mar 4, 2016
Merged

Conversation

aquette
Copy link
Member

@aquette aquette commented Mar 1, 2016

Any variable that is not STRING, RANGE or ENUM is just a simple numeric value.
The protocol documentation (net-protocol.txt) was previously stating that "The
default , when omitted, is integer." which was not fully true, since a
variable could also be a float. Hence, the wording was changed to advertise
this, and that each driver is then responsible for handling values as either
integer or float. Moreover, instead of returning a TYPE "UNKNOWN", return
"NUMERIC", which is more suitable, and aligned with the NUT protocol
specification

Any variable that is not STRING, RANGE or ENUM is just a simple numeric value.
The protocol documentation (net-protocol.txt) was previously stating that "The
default <type>, when omitted, is integer." which was not fully true, since a
variable could also be a float.  Hence, the wording was changed to advertise
this, and that each driver is then responsible for handling values as either
integer or float.  Moreover, instead of returning a TYPE "UNKNOWN", return
"NUMERIC", which is more suitable, and aligned with the NUT protocol
specification
@aquette aquette added this to the 2.7.4 milestone Mar 1, 2016
@aquette
Copy link
Member Author

aquette commented Mar 1, 2016

calling for approval @clepple , @zykh and @balooloo, since it's touching the net-protocol, even if it's just a fix

@EmilienKia
Copy link
Contributor

Nothing against this proposal.

Just one colateral point: as you "formalize" the fact to be able to deal with integers or floats, perhaps we have to specify which numeric formats are allowed (only decimal english-based representation ?).

Clarify documentation on how to express float values, when using upsrw.  That is
to say, using decimal english-based representation, so using a dot
@aquette
Copy link
Member Author

aquette commented Mar 2, 2016

@balooloo : indeed, thanks for the comments. I've just committed (52e9957) some doc clarification, both in the net-protocol, and in upsrw manpage.
Good to go?

@EmilienKia
Copy link
Contributor

Yes

@aquette aquette self-assigned this Mar 2, 2016
@zykh
Copy link
Contributor

zykh commented Mar 3, 2016

mmh..

  • Is there any restriction for the supported formats in 'NUMERIC' vars? For example would 1, 2.3, 4.5e+6, 7a, 0x7b, ... be valid formats or we only have to support base 10 in non-scientific notations? If so, we should state it.
  • Not sure about this, but maybe 'NUMBER' would fit better than 'NUMERIC'... like: "What's that var? It's a NUMBER."

Given that we're opening the Pandora's jar:

  • I know that most of our vars are numbers and so it's easier to only edit the type for the few string ones, but maybe it would be better to have 'STRING' as the default type, since a number can always be stored in a string (well.. not always true, but pretend it to be), but not viceversa: if, for some reason, we lie (like me forgetting to set a var as the correct base type STRING/NUMERIC), then if the default type is STRING and the var is not, we're not doing something as bad as if the default type were NUMERIC and the var a string.
  • Looking quickly at netget.c's get_type() (and the same thing applies to upsrw): if a var is an ENUM or RANGE, then the base type (STRING/NUMERIC) is not printed: RANGE implies NUMERIC so we could directly print it if node->range_list exists, while ENUMs should be splitted between STRING and NUMERIC.

As per discussion on the Github pull request, NUMBER would be more suitable than
NUMERIC
Clarify a bit more documentation on how to express float values, when using
upsrw.  That is to say, using decimal (base 10) english-based representation, so
using a dot, in non-scientific notation.  So hexadecimal, exponents, and comma
for thousands separator are forbiden
@aquette
Copy link
Member Author

aquette commented Mar 3, 2016

@zykh : true for both non-scientific notation and NUMBER Vs NUMERIC. I've amended with 2 commits
As for the 2 others "pandora's jar":

  • defaulting to string would be interesting, but would require to define explicitly a ST_FLAG_NUMBER and to apply it everywhere (where there is no ST_FLAG_STRING) to be useful.
  • true for netget / upsrw: the mod is easy for netget, but requires code refactoring for upsrw. I would be in favor of a separate ticket, for 2.7.5, if you don't mind, not to delay much more 2.7.4
    Would it be ok for you

@zykh
Copy link
Contributor

zykh commented Mar 3, 2016

@aquette: sure, those things can wait 2.7.5

@aquette aquette changed the title Default to type NUMERIC for variables Default to type NUMBER for variables Mar 4, 2016
@aquette
Copy link
Member Author

aquette commented Mar 4, 2016

Create follow-up tickets #266 and #267.
Closing the present one

@aquette aquette closed this Mar 4, 2016
@aquette aquette reopened this Mar 4, 2016
@aquette
Copy link
Member Author

aquette commented Mar 4, 2016

hem, forgot that there was a branch to merge...

@aquette aquette merged commit f3541e4 into master Mar 4, 2016
aquette added a commit that referenced this pull request Mar 4, 2016
@aquette aquette deleted the default_type_numeric branch March 7, 2016 15:42
jimklimov added a commit to jimklimov/nut that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants