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

Adding 'json/flat' formatter #145

Closed
wants to merge 1 commit into from
Closed

Adding 'json/flat' formatter #145

wants to merge 1 commit into from

Conversation

ericbreno
Copy link

@ericbreno ericbreno commented Oct 2, 2018

Issue: #138

Description of changes: Added the 'json/flat' formatter to file formats.js. Simple recursive strategy to build up keys with it's path and evaluate values as strings directly or as a reference to another value in dictionary input object.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

Just a couple of comments.

'json/flat': function(dictionary) {
var evaluateProp = function(property) {
// property value is in format '{*}'
var isReference = /^\{.*\}/g.test(property);
Copy link
Contributor

Choose a reason for hiding this comment

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

there is an open discussion if continue supporting multiple references in the same token value or not (see #118). in theory, the value could be something like {abc.def} {ghi.jkl} in that case the code may not work as expected.
@dbanksdesign suggestions here? what do we do?

Copy link
Author

Choose a reason for hiding this comment

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

If it's needed to keep support, I just need to know how it should work. Waiting.

Copy link
Member

Choose a reason for hiding this comment

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

The style dictionary system already handles the references at this point before getting to the formatter, so you should not be trying to resolve the reference. I think the confusion here is in the example I gave in the issue is referencing the input JSON to a style dictionary, which gets processed by the style dictionary before being passed to a formatter. Take a look at https://amzn.github.io/style-dictionary/#/build_process to see what is happening. Apologies for any confusion.

// recursive, initial prefix is none (empty string)
// iterate over keys capitalizing and concatenating them
// until "value" key is found, then evaluate it's value
var deepFlatten = function(object, prefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably I am missing something, but I think that at this point in the pipeline you already have the prop.name and the prop.value (and potentially also the property.path), no? can you compare with other formats and see if they may be of help?

Copy link
Author

Choose a reason for hiding this comment

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

Didn't get it. Where I'd have prop.name or prop.path? There's another pre-processor for JSON that can do this?
In this code I'm iterating recursively (like a DFS) and also saving the actual path until now to build the final path/name for each property

Copy link
Member

Choose a reason for hiding this comment

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

@didoo is correct, see my comment above about Style Dictionary already processing the input JSON. The formatter function takes 2 arguments, a dictionary and a config. Both are objects, the important one for this is the dictionary which has 2 attributes: properties and allProperties. allProperties is already a flattened list of the design tokens (properties) which have already been processed. If you take a look at the 'javascript/es6' format you can see this in action.

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

Apologies for the confusion, the Style Dictionary system does JSON processing on input files before sending them to a formatter. Take a look at the documentation to see what is happening and take a look at other 'flat' formatters like 'javascript/es6', 'scss/variables', 'less/variables'.

* @kind member
* @example
* ```json
{
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to have asterisks at the beginning of these lines because they need to be part of the comment block. Same for the other JSON examples in the comment below

'json/flat': function(dictionary) {
var evaluateProp = function(property) {
// property value is in format '{*}'
var isReference = /^\{.*\}/g.test(property);
Copy link
Member

Choose a reason for hiding this comment

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

The style dictionary system already handles the references at this point before getting to the formatter, so you should not be trying to resolve the reference. I think the confusion here is in the example I gave in the issue is referencing the input JSON to a style dictionary, which gets processed by the style dictionary before being passed to a formatter. Take a look at https://amzn.github.io/style-dictionary/#/build_process to see what is happening. Apologies for any confusion.

// recursive, initial prefix is none (empty string)
// iterate over keys capitalizing and concatenating them
// until "value" key is found, then evaluate it's value
var deepFlatten = function(object, prefix) {
Copy link
Member

Choose a reason for hiding this comment

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

@didoo is correct, see my comment above about Style Dictionary already processing the input JSON. The formatter function takes 2 arguments, a dictionary and a config. Both are objects, the important one for this is the dictionary which has 2 attributes: properties and allProperties. allProperties is already a flattened list of the design tokens (properties) which have already been processed. If you take a look at the 'javascript/es6' format you can see this in action.

describe('formats', function () {
describe('json/flat should', function () {
it('evaluate complex object with property referencing another value', function () {
var dictionary = {
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at test/formats/scssVariables.js for a better example how to test this formatter.

@chazzmoney
Copy link
Collaborator

@ericbreno Are you still pursuing this? Let us know if you need additional pointers.

@chazzmoney
Copy link
Collaborator

Closing as there has been no movement. Feel free to re-open if you have new changes.

@chazzmoney chazzmoney closed this Nov 2, 2018
@didoo
Copy link
Contributor

didoo commented Nov 2, 2018

@chazzmoney I am picking up this one (we need a json/flat format among the pre-defined formats)

@didoo didoo mentioned this pull request Nov 2, 2018
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