vtrushin / json-to-ast

JSON AST parser
MIT License
237 stars 36 forks source link

Some of `type property` nodes have key only containing `position` when verbose true (default) #5

Closed tunnckoCore closed 7 years ago

tunnckoCore commented 8 years ago

1) Not make sense, absolutely. This inconsistency around is not good.

{ type: 'property',
  key: 
   { type: 'key',
     value: 'foo',
     position: { start: [Object], end: [Object], human: '2:3 - 2:6 [4:7]' } },
  value: 
   { type: 'string',
     value: 'sa',
     position: { start: [Object], end: [Object], human: '2:7 - 2:11 [8:12]' } } }
{ type: 'property',
  key: { position: { start: [Object], end: [Object], human: '3:3 - 3:9 [16:22]' } },
  value: 
   { type: 'true',
     value: null,
     position: { start: [Object], end: [Object], human: '3:11 - 3:15 [24:28]' } } }
{ type: 'property',
  key: { position: { start: [Object], end: [Object], human: '4:3 - 4:9 [32:38]' } },
  value: 
   { type: 'string',
     value: 'hyge',
     position: { start: [Object], end: [Object], human: '4:11 - 4:17 [40:46]' } } }

How we'll know what's the name of the key that contains true value? How we'll know what's the name of the key that contains string with value hyge (the third/last node)? Btw, in that second node here also have something wrong. Value of type: 'true' is null (same applies for type: false)? Not make sense.

It's correct/valid only for type: null

{ type: 'null',
  value: null,
  position: 
   { start: { line: 22, column: 10, char: 263 },
     end: { line: 22, column: 14, char: 267 },
     human: '22:10 - 22:14 [263:267]' } }

* maybe make sense to rename type of the null, true and false to literal so

{
  type: 'literal'
  value: true
}
{
  type: 'literal'
  value: null
}
{
  type: 'literal'
  value: false
}

2) Also, it would be better to rename position[start/end].char to offset.

3) Third inconsistency is that .properties and .items thing. It's not comfortable for walkers and etc - you always should check the .type of the coming node.

You may also be interested in https://github.com/finnp/json-lexer. Try to use it? And we should implement https://github.com/finnp/json-lexer/issues/3 (positions).

tunnckoCore commented 8 years ago

I need stable AST tree, because of the @postjson / @postcore things (they will support custom parsers, but we need default stable one for valid/strict json). So we need consistency and everything else would be plugin. Easiest way is to use JSON.parse to build the AST, but the biggest issue with using it is that it push to you values of the object, before the object key.

For example

var json = `{
  "foo": "bar baz",
  "scripts": {
    "test": "standard && node test.js",
    "qux": "zzz"
  },
  "xx": true
}
`

JSON.parse(json, function (key, value) {
  if (key === '') return
  console.log(key, value)
})
// =>
// foo bar baz
// test standard && node test.js
// qux zzz
// scripts {}
// xx true

Notice the test and qux keys are before scripts? That's the problem.

tunnckoCore commented 8 years ago

It going even worse if it have more nested objects.

var json = `{
  "foo": "bar baz",
  "scripts": {
    "test": "standard && node test.js",
    "qux": "zzz",
    "nested": {
      "rules": "okey",
      "or": "not okey"
    }
  },
  "xx": true
}
`

JSON.parse(json, function (key, value) {
  if (key === '') return
  console.log(key, value)
})
// =>
// foo bar baz
// test standard && node test.js
// qux zzz
// rules okey
// or not okey
// nested {}
// scripts {}
// xx true
tunnckoCore commented 8 years ago

Btw, I'm working on some AST, and accomplished it with patch to reviver of default JSON.parse. Only implementation for positions are missing currently, but i'm digging around few lexer/parsers (and yours), so can done it soon.

https://github.com/tunnckoCore/ideas/issues/33

vtrushin commented 8 years ago

I'm sorry I did not answer for a long time

How we'll know what's the name of the key that contains true value?

It's strange that property hasn't key value. Can you post your json sample?

{
  type: 'true | false | string | number | null',
  value: null
}

It's really not obvious that I will change it soon

Also, it would be better to rename position[start/end].char to offset

Yes, I too agree

Third inconsistency is that .properties and .items thing. It's not comfortable for walkers and etc - you always should check the .type of the coming node.

Do not quite understand. These are different types and you need to work with them in different ways. Of course you need to check

You may also be interested in https://github.com/finnp/json-lexer. Try to use it?

Yes, it's look interesting. But I didn't use it

tunnckoCore commented 8 years ago

I'm sure that there's something totally wrong.

var parse = require('json-to-ast')
var str = `{
  "foo": "sa",
  "bar": "true",
  "bool": false,
  "qux": null
}`

console.log(parse(str).properties)

yields

[ { type: 'property',
    key: { type: 'key', value: 'foo', position: [Object] },
    value: { type: 'string', value: 'sa', position: [Object] } },
  { type: 'property',
    key: { position: [Object] },
    value: { type: 'string', value: 'true', position: [Object] } },
  { type: 'property',
    key: { position: [Object] },
    value: { type: 'false', value: null, position: [Object] } },
  { type: 'property',
    key: { position: [Object] },
    value: { type: 'null', value: null, position: [Object] } } ]
  1. It's totally wrong value of type true to be null, it only make sense when type is "null".
  2. As you can see, only the first one have correct key object - key.type, key.value, key.position, others have key object containing only position which is total kill of data

Try this one (just set different thing as first key/value pair)

var parse = require('json-to-ast')
var str = `{
  "qux": null,
  "foo": "sa",
  "bar": "true",
  "bool": false
}`

console.log(parse(str).properties)

notice the result

[ { type: 'property',
    key: { type: 'key', value: 'qux', position: [Object] },
    value: { type: 'null', value: null, position: [Object] } },
  { type: 'property',
    key: { position: [Object] },
    value: { type: 'string', value: 'sa', position: [Object] } },
  { type: 'property',
    key: { position: [Object] },
    value: { type: 'string', value: 'true', position: [Object] } },
  { type: 'property',
    key: { position: [Object] },
    value: { type: 'false', value: null, position: [Object] } } ]

this is total proof that something is wrong with setting key object - it always only the first property object has correct key object, others only have the position object.

These are different types and you need to work with them in different ways. Of course you need to check

It's uncomfortable for user and plugins perspective and walker functions. See retext, remark, postcss. It's just enough they all to have children instead of different names for same thing - they really are childrens.

* maybe make sense to rename type of the null, true and false to literal so

would be really good thing.

vtrushin commented 8 years ago

As you can see, only the first one have correct key object - key.type, key.value, key.position, others have key object containing only position which is total kill of data

Fixed at 1.2.13. Your example: https://tonicdev.com/57c427eda02ae615006d25c0/57d0062b6e551c140095024f