Skip to content

Conversation

@Darxo
Copy link
Contributor

@Darxo Darxo commented Feb 17, 2024

Closes: #335

@Darxo Darxo force-pushed the feat-requireUnsigned branch from 7e4e034 to e785dae Compare February 17, 2024 18:49
{
if (value < 0)
{
::logError(value + " must have the type: unsigned");
Copy link
Member

Choose a reason for hiding this comment

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

I'd argue unsigned isn't actually a type and therefore the error should be something more like.

value + " must have the type: integer, and be greater than or equal to 0"

Copy link
Member

Choose a reason for hiding this comment

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

In that same vein what about the function name? Should it be something like requireUInt?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe that's better too

@Enduriel Enduriel added this to the 1.4.0 milestone Feb 27, 2024
@TaroEld
Copy link
Member

TaroEld commented Mar 1, 2024

Hm, it's not really an uint; maybe just call it "requirePositive"?


::MSU.requireUnsigned <- function( ... )
{
::MSU.requireTypeArray("integer", vargv);
Copy link
Contributor Author

@Darxo Darxo Mar 4, 2024

Choose a reason for hiding this comment

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

I will say this use of the existing require check only seemed good back then.
But in reality this means that failing an unsigned integer check will print a "failed an integer check" message half the time. And that is not fully correct.

Ideally we would copy&paste the integer check logic in this check but swap out the printed exception

@Darxo Darxo requested a review from Enduriel March 6, 2024 11:53
@LordMidas LordMidas force-pushed the development branch 2 times, most recently from 6677ac8 to ae825b7 Compare June 17, 2025 23:43
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.

4 participants