KittyGiraudel / ama

Ask me anything!
43 stars 4 forks source link

Error handling in sass: am I doing it right? #111

Closed ghost closed 4 years ago

ghost commented 4 years ago

Hi,

I have a small question for you. Currently I am building a very small utilities mixin library in sass to publish it later. For handling common incorrect input argument on the mixins (numeric values and colors mostly) I did something like this:

// core of the handler (return error string if it find an error)
@function validate-input($numeric-values:null, $colors:null) {

  @each $numeric-value in $numeric-values {

    @if type-of($numeric-value) !=number and not index(initial inherit null auto, $numeric-value) {
      @return "The argument given as input must be a valid numeric-value, currently it is `#{$numeric-value}` (#{type-of($numeric-value)}), please check your input";
    }

  }

  @each $color in $colors {

    @if type-of($color) !=color and not index(initial inherit null auto, $color) {
      @return "The argument given as input must be a valid color, currently it is `#{$color}` (#{type-of($color)}), please check your input";
    }

  }

  @return false;
}

// simple mixin that act like a bridge between mixins and the function
@mixin validate-input($numeric-values:null, $colors:null) {

  $error-check: validate-input($numeric-values, $colors);

  @if $error-check {
    @error $error-check;
  }

}

// simple integration example 

@mixin line($width, $height, $color) {
// integrate the error handler
  @include validate-input($width $height, $color);

  content: "";
  display: block;
  @include size($width, $height);
  background-color: $color;
}

What do you thing about this method? Is a to aggressive method? Should avoid it and do another simpler method?

Thank you

KittyGiraudel commented 4 years ago

Hello!

I think it’s pretty good, although I would tackle it this way:


$ALLOWED_KEYWORDS: ('initial', 'inherit',  'auto', null);

@mixin ensure-type($type, $value) {
  @if type-of($value) != $type and not index($ALLOWED_KEYWORDS, $value) {
    @error "The argument given as input must be a valid #{$type}, currently it is `#{$value}` (#{type-of($value)}), please check your input";
  }
}

@mixin ensure-number($value) {
  @include ensure-type('number', $value);
}

@mixin ensure-color($value) {
  @include ensure-type('color', $value);
}

@mixin size($width, $height) {
  @include ensure-number($width);
  @include ensure-number($height);

  width: $width;
  height: $height;
}

@mixin line($width, $height, $color) {
  @include ensure-color($color);
  @include size($width, $height);
  content: "";
  display: block;
  background-color: $color;
}

.foo {
  @include line(10, 20px, 'red');
}
ghost commented 4 years ago

Hi, thank you very much for your I think, that your implementation is plenty more flexible (you can check all the types you want) and well organized (without functions and necessary bridge).

The only downside about it, Is that you need for each value and type a @include directive, and it becomes uncomfortable when you have a lot of arguments passed or you need to refactor the mixins very frequently.

What do you think about this version of your implementation instead?

@mixin ensure-type($args...) {

  $allowed-keywords: ('initial', 'inherit', 'auto', null);
  $data-types: (number, string, color, list, map, bool, null);
  $type: null;

// take the type from the arguments
  @each $arg in $args {

    @if index($data-types, $arg) {
      $type: $arg;
    }

    @else {
 // core 
      @if type-of($arg) !=$type and not index($allowed-keywords, $arg) {
        @error "The argument given as input must be a `#{$type}`, currently it is `#{$arg}` with a data type of `#{type-of($arg)}`, please check your input";
      }
    }

  }
}
// example of integration

@mixin line($width, $height, $color) {
// Ensure that width and height are valid number and color is a valid color
  @include ensure-type(number, $width, $height, color, $color);

  content: "";
  display: block;
 width: $width;
height: $height;
  background-color: $color;
}

With this version you can check multiple types and values at once.

ghost commented 4 years ago

Little update:

//old
 $data-types: (number, string, color, list, map, bool, null);
//new
 $data-types: (number, string, color, list, map, bool);

Because if the user insert a null value like argument, the mixin will detect that the type to check is null.

KittyGiraudel commented 4 years ago

I‘d recommend using a map instead of a list:

@include ensure-types((
  'number': ($width, $height),
  'color': $color
));
ghost commented 4 years ago

Oh, very good idea, it hadn't occurred to me.

ghost commented 4 years ago

thank you very much for all the help that you have given to me. You can consider the issue closed!