Support for maps properties #25

Closed
pascalduez opened this Issue Jun 27, 2014 · 23 comments

Comments

Projects
None yet
4 participants
@pascalduez
Member

pascalduez commented Jun 27, 2014

Might be useful to have a syntax to document maps, a per key/val documenting.
For instance configuration maps.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jun 27, 2014

Member

We thought about that in #15.

This is too soon to implement it, but definitely not too soon to talk about it so I am willing to consider any proposal.

Member

HugoGiraudel commented Jun 27, 2014

We thought about that in #15.

This is too soon to implement it, but definitely not too soon to talk about it so I am willing to consider any proposal.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 3, 2014

Member

First, let's deal with #36.

Member

HugoGiraudel commented Jul 3, 2014

First, let's deal with #36.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 5, 2014

Member

I want everyone's opinion on how we should document maps. Go. :)

Member

HugoGiraudel commented Jul 5, 2014

I want everyone's opinion on how we should document maps. Go. :)

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 5, 2014

Member

Following @valeriangalliat proposition on #15 , I guess in place keys commenting would be neat.
Something like @key or more explicit @property ?
@property == @key: @default-val ...

/**
 * Default settings.
 * ---
 * @type Map
 */
$settings: (
  base: (
    default: (
      /**
       * @prop {Color}
       * description
       */
      text: black,

      /**
       * @prop {Number}
       * description
       */
      size: 50px,

      /**
       * @prop {String}
       * description
       */
      selector: widget,
    ),
  ),
);
Member

pascalduez commented Jul 5, 2014

Following @valeriangalliat proposition on #15 , I guess in place keys commenting would be neat.
Something like @key or more explicit @property ?
@property == @key: @default-val ...

/**
 * Default settings.
 * ---
 * @type Map
 */
$settings: (
  base: (
    default: (
      /**
       * @prop {Color}
       * description
       */
      text: black,

      /**
       * @prop {Number}
       * description
       */
      size: 50px,

      /**
       * @prop {String}
       * description
       */
      selector: widget,
    ),
  ),
);
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 5, 2014

Member

Considered and rejected. I don't want comments that enforce a specific coding syntax.

Member

HugoGiraudel commented Jul 5, 2014

Considered and rejected. I don't want comments that enforce a specific coding syntax.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 5, 2014

Member

I don't want comments that enforce a specific coding syntax.

You mean forcing authors to format their maps multi-lines ?

Member

pascalduez commented Jul 5, 2014

I don't want comments that enforce a specific coding syntax.

You mean forcing authors to format their maps multi-lines ?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 5, 2014

Member

You mean forcing authors to format their maps multi-lines ?

I think most authors pretty print their maps so that's no concern here. I mean that I don't want comments to be injected in the code. Comments should come before a declaration, not in the middle of it.

Member

HugoGiraudel commented Jul 5, 2014

You mean forcing authors to format their maps multi-lines ?

I think most authors pretty print their maps so that's no concern here. I mean that I don't want comments to be injected in the code. Comments should come before a declaration, not in the middle of it.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 5, 2014

Member

Comments should come before a declaration, not in the middle of it.

Yep, I understand that.

Then why not following the function/mixin way ?

/**
 * Default settings.
 * ---
 * @type Map
 * ---
 * @prop base - description
 * -- @prop default - description
 * ---- @prop {Color} text (black) - description
 * ---- @prop {Number} size (50px) - description
 * ---- @prop {String} selector (widget) - description
 */
$settings: (
  base: (
    default: (
      text: black,
      size: 50px,
      selector: widget,
    ),
  ),
);

The issue is with how to signify nesting. Does parsing spaces/white-space is reliable enough ?

Member

pascalduez commented Jul 5, 2014

Comments should come before a declaration, not in the middle of it.

Yep, I understand that.

Then why not following the function/mixin way ?

/**
 * Default settings.
 * ---
 * @type Map
 * ---
 * @prop base - description
 * -- @prop default - description
 * ---- @prop {Color} text (black) - description
 * ---- @prop {Number} size (50px) - description
 * ---- @prop {String} selector (widget) - description
 */
$settings: (
  base: (
    default: (
      text: black,
      size: 50px,
      selector: widget,
    ),
  ),
);

The issue is with how to signify nesting. Does parsing spaces/white-space is reliable enough ?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 5, 2014

Member

Then why not following the function/mixin way ?

That's the spirit.

Does parsing spaces/white-space is reliable enough ?

I'd like to avoid that.

Member

HugoGiraudel commented Jul 5, 2014

Then why not following the function/mixin way ?

That's the spirit.

Does parsing spaces/white-space is reliable enough ?

I'd like to avoid that.

@HugoGiraudel HugoGiraudel added this to the 1.2 milestone Jul 5, 2014

@HugoGiraudel HugoGiraudel added the 1.2 label Jul 5, 2014

@HugoGiraudel HugoGiraudel self-assigned this Jul 5, 2014

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Jul 5, 2014

Member

I like @pascalduez approach but would ultimately think that this is much more clean

/**
 * Default settings.
 * ---
 * @type Map
 * ---
 * @prop base - description
 * @prop base.default - description
 * @prop {Color} base.default.text (black) - description
 * @prop {Number} base.default.size (50px) - description
 * @prop {String} base.default.selector (widget) - description
 */
$settings: (
  base: (
    default: (
      text: black,
      size: 50px,
      selector: widget,
    ),
  ),
);

This buts the whole context in every line. So if you search for a specify thing the context is always there. Without the need to know in which context the line was.

Member

FWeinb commented Jul 5, 2014

I like @pascalduez approach but would ultimately think that this is much more clean

/**
 * Default settings.
 * ---
 * @type Map
 * ---
 * @prop base - description
 * @prop base.default - description
 * @prop {Color} base.default.text (black) - description
 * @prop {Number} base.default.size (50px) - description
 * @prop {String} base.default.selector (widget) - description
 */
$settings: (
  base: (
    default: (
      text: black,
      size: 50px,
      selector: widget,
    ),
  ),
);

This buts the whole context in every line. So if you search for a specify thing the context is always there. Without the need to know in which context the line was.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 5, 2014

Member

I really like this. No reason not to include the type type when it's a nested map though. That being said, we could make it optional as well.

/**
 * Default settings.
 * ---
 * @type Map
 * ---
 * @prop {Map} base - description
 * @prop {Map} base.default - description
 * @prop {Color} base.default.text (black) - description
 * @prop {Number} base.default.size (50px) - description
 * @prop {String} base.default.selector (widget) - description
 */
$settings: (
  base: (
    default: (
      text: black,
      size: 50px,
      selector: widget,
    )
  )
);
Member

HugoGiraudel commented Jul 5, 2014

I really like this. No reason not to include the type type when it's a nested map though. That being said, we could make it optional as well.

/**
 * Default settings.
 * ---
 * @type Map
 * ---
 * @prop {Map} base - description
 * @prop {Map} base.default - description
 * @prop {Color} base.default.text (black) - description
 * @prop {Number} base.default.size (50px) - description
 * @prop {String} base.default.selector (widget) - description
 */
$settings: (
  base: (
    default: (
      text: black,
      size: 50px,
      selector: widget,
    )
  )
);
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 7, 2014

Member

I am okay with this version.

Member

HugoGiraudel commented Jul 7, 2014

I am okay with this version.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Jul 7, 2014

Member

Looks neat. And yes, optional type would be better.

Member

pascalduez commented Jul 7, 2014

Looks neat. And yes, optional type would be better.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 7, 2014

Member

In the meantime, I suggest to those of you who want to document maps to use @pascalduez's trick: http://pascalduez.github.io/SassyIcons/docs/#variable-icons-defaults.

Tl;dr: use inline comments within the map so they get grabbed and printed as regular content.

Member

HugoGiraudel commented Jul 7, 2014

In the meantime, I suggest to those of you who want to document maps to use @pascalduez's trick: http://pascalduez.github.io/SassyIcons/docs/#variable-icons-defaults.

Tl;dr: use inline comments within the map so they get grabbed and printed as regular content.

@alademann

This comment has been minimized.

Show comment
Hide comment
@alademann

alademann Jul 7, 2014

Contributor

+1 for using dot notation to imply nesting levels!

I actually use a custom get Sass function in my Sass project that explodes dot-notated references to deeply nested map properties... just extends atop SassyMap's map-get-deep leveraging SassyList's sl-explode function...

/**
 * Fetch a deeply nested value in a multi-level map using object dot-notation string OR a list of keys.
 *
 * @requires sassy-maps
 * @requires SassyLists
 * @requires is-map
 * @requires is-string
 * @requires is-list
 *
 * @param {map}           $map
 * @param {string | list} $keys         object dot-notation string representing the nesting order of the desired key
 * @param {string}        $delimiter    string used to identify the individual keys within the string
 *
 * @throws if any of the `$required-functions` do not exist
 * @throws if `$map` param is not a map
 * @throws if `$keys` param is not a string or a list
 *
 * @return {*} value at nesting level requested via `$keys` param
 */
@function get($map, $keys, $delimiter: '.') {
    $required-functions: is-map, is-string, is-list, sl-explode, map-get-deep;
    @each $function in $required-functions {
        @if not function-exists($function) {
            @warn "`#{$function}()` is required by `get()`.";
            @return null;
        }
    }
    @if not is-map($map) {
        @warn "`get` function expecting a map; #{type-of($map)} given.";
        @return null;
    }
    @if not is-string($keys) and not is-list($keys) {
        @warn "`get` function expecting a list or a string; #{type-of($keys)} given.";
        @return null;
    }
    // END error-checking

    @if is-list($keys) {
        //
        // its already a list, no need to explode it
        //
        @return map-get-deep($map, $keys);
    } @else {
        @if not str-index($keys, $delimiter) {
            //
            // a single key has been requested - no recursion necessary
            // (no `$delimiter` found in `$keys`)
            //
            @return map-get($map, $keys);
        } @else {
            //
            // a `$delimiter` was found in the `$keys` string
            // lets split it on the delimiter so we can pass a valid list of keys to `map-get-deep()`
            //
            $key-list: sl-explode($keys, $delimiter);

            @return map-get-deep($map, $key-list)
        }
    }
}
.btn {
  padding: get($btn-config, 'padding.base.all');
  font-size: get($btn-config, 'text.font-size.base');
  color: get($btn-config, 'text.color.base');

  &:hover,
  &:focus {
      color: get($btn-config, 'text.color.hover');
  }

  &.btn-xs {
      padding: get($btn-config, 'padding.xs.all');
      font-size: get($btn-config, 'text.font-size.xs');
  }

  // etc...
}
Contributor

alademann commented Jul 7, 2014

+1 for using dot notation to imply nesting levels!

I actually use a custom get Sass function in my Sass project that explodes dot-notated references to deeply nested map properties... just extends atop SassyMap's map-get-deep leveraging SassyList's sl-explode function...

/**
 * Fetch a deeply nested value in a multi-level map using object dot-notation string OR a list of keys.
 *
 * @requires sassy-maps
 * @requires SassyLists
 * @requires is-map
 * @requires is-string
 * @requires is-list
 *
 * @param {map}           $map
 * @param {string | list} $keys         object dot-notation string representing the nesting order of the desired key
 * @param {string}        $delimiter    string used to identify the individual keys within the string
 *
 * @throws if any of the `$required-functions` do not exist
 * @throws if `$map` param is not a map
 * @throws if `$keys` param is not a string or a list
 *
 * @return {*} value at nesting level requested via `$keys` param
 */
@function get($map, $keys, $delimiter: '.') {
    $required-functions: is-map, is-string, is-list, sl-explode, map-get-deep;
    @each $function in $required-functions {
        @if not function-exists($function) {
            @warn "`#{$function}()` is required by `get()`.";
            @return null;
        }
    }
    @if not is-map($map) {
        @warn "`get` function expecting a map; #{type-of($map)} given.";
        @return null;
    }
    @if not is-string($keys) and not is-list($keys) {
        @warn "`get` function expecting a list or a string; #{type-of($keys)} given.";
        @return null;
    }
    // END error-checking

    @if is-list($keys) {
        //
        // its already a list, no need to explode it
        //
        @return map-get-deep($map, $keys);
    } @else {
        @if not str-index($keys, $delimiter) {
            //
            // a single key has been requested - no recursion necessary
            // (no `$delimiter` found in `$keys`)
            //
            @return map-get($map, $keys);
        } @else {
            //
            // a `$delimiter` was found in the `$keys` string
            // lets split it on the delimiter so we can pass a valid list of keys to `map-get-deep()`
            //
            $key-list: sl-explode($keys, $delimiter);

            @return map-get-deep($map, $key-list)
        }
    }
}
.btn {
  padding: get($btn-config, 'padding.base.all');
  font-size: get($btn-config, 'text.font-size.base');
  color: get($btn-config, 'text.color.base');

  &:hover,
  &:focus {
      color: get($btn-config, 'text.color.hover');
  }

  &.btn-xs {
      padding: get($btn-config, 'padding.xs.all');
      font-size: get($btn-config, 'text.font-size.xs');
  }

  // etc...
}

@HugoGiraudel HugoGiraudel added 1.3 and removed 1.2 labels Jul 19, 2014

@HugoGiraudel HugoGiraudel modified the milestones: 1.3, 1.2 Jul 19, 2014

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 19, 2014

Member

Moving this to 1.3.

Member

HugoGiraudel commented Jul 19, 2014

Moving this to 1.3.

@HugoGiraudel HugoGiraudel assigned FWeinb and unassigned HugoGiraudel and FWeinb Jul 19, 2014

@HugoGiraudel HugoGiraudel removed the 1.3 label Jul 30, 2014

@FWeinb FWeinb modified the milestones: 1.3, 1.2 Aug 1, 2014

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Aug 1, 2014

Member

Sorry, clicked the wrong thing.

Member

FWeinb commented Aug 1, 2014

Sorry, clicked the wrong thing.

@FWeinb FWeinb modified the milestones: 1.2, 1.3 Aug 1, 2014

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 13, 2014

Member

Next and last for 1.3.

Member

HugoGiraudel commented Aug 13, 2014

Next and last for 1.3.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 18, 2014

Member

What about this? I release 1.3, we delay this to 1.4?

Member

HugoGiraudel commented Aug 18, 2014

What about this? I release 1.3, we delay this to 1.4?

@HugoGiraudel HugoGiraudel removed this from the 1.3 milestone Aug 18, 2014

@HugoGiraudel HugoGiraudel added this to the 1.4 milestone Aug 18, 2014

@HugoGiraudel HugoGiraudel modified the milestones: 1.4, 1.5 Aug 20, 2014

FWeinb added a commit that referenced this issue Aug 20, 2014

@FWeinb FWeinb referenced this issue Aug 20, 2014

Merged

Implement #25 #180

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 20, 2014

Member

@pascalduez We are very close to ship this. Do you have a specific idea on how you'd like to display a map structure in the view? A table or something? I was thinking of the exact same way as parameters.

Member

HugoGiraudel commented Aug 20, 2014

@pascalduez We are very close to ship this. Do you have a specific idea on how you'd like to display a map structure in the view? A table or something? I was thinking of the exact same way as parameters.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Aug 21, 2014

Member

Yeah, I guess cloning the parameter presentation is the way to go for now, so we don't ends up with too many different structures.

Member

pascalduez commented Aug 21, 2014

Yeah, I guess cloning the parameter presentation is the way to go for now, so we don't ends up with too many different structures.

@FWeinb

This comment has been minimized.

Show comment
Hide comment
@FWeinb

FWeinb Aug 21, 2014

Member

👍

Member

FWeinb commented Aug 21, 2014

👍

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 24, 2014

Member

Merged in develop.

Member

HugoGiraudel commented Aug 24, 2014

Merged in develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment