Skip to content

Define positive and non-null number field types#251

Merged
aababilov merged 3 commits intogoogle:masterfrom
aababilov:positive-integer
Jan 4, 2021
Merged

Define positive and non-null number field types#251
aababilov merged 3 commits intogoogle:masterfrom
aababilov:positive-integer

Conversation

@aababilov
Copy link
Contributor

They are already used in the table descriptions.

They are already used in the table descriptions.
Copy link
Contributor

@timMillet timMillet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!

If you want to be exhaustive with this PR, "Float" alone should be also defined as it is used for levels.level_index and pathways.max_slope.

@sccmcca
Copy link
Contributor

sccmcca commented Dec 3, 2020

I'm wondering if we could separate Field Types from their factors as to avoid exhaustively defining every factor combination of integer and float (and potentially currency amount). Also, "Non-null" could be interpreted as "not empty". I think the intended meaning is "Non-zero". Something like:

Field Types

  • ...
  • Float - A floating point number.
  • Integer - An integer.
  • ...

Field Factors

Factors applicable to Float or Integer field types:

  • Non-zero - Non-zero.
  • Non-negative - Greater than or equal to zero.
  • Positive - Greater than zero.

@timMillet
Copy link
Contributor

I like the idea!
However, each field type and field factor should have a precise definition (e.g: "Non-zero - Non-zero" is not a precise definition).

@sccmcca
Copy link
Contributor

sccmcca commented Dec 11, 2020

@timMillet @aababilov Please see the PR I made (aababilov#1) containing my revised suggestion as outlined in my previous comment. If it does not address the need, feedback for improvement is welcomed!

Add field signs section
@aababilov
Copy link
Contributor Author

Updated. PTAL.

@aababilov aababilov requested a review from timMillet December 22, 2020 22:53
Copy link
Contributor

@timMillet timMillet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good for me!

@sccmcca
Copy link
Contributor

sccmcca commented Jan 4, 2021

@aababilov @timMillet I forgot a detail. As Field Signs is a new heading, I suggest to add it to the TOC. See my PR here. Thoughts?

Add TOC for Field Signs
@aababilov aababilov merged commit e60c620 into google:master Jan 4, 2021
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.

3 participants