Skip to content

Conversation

@markborkum
Copy link
Contributor

Implements length() function for JSON arrays, and entries(), keys() and values() functions for JSON objects.

Description

n/a

Issues Resolved

n/a

Check List

  • All tests pass.
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable

Implements `length()` function for JSON arrays, and `entries()`, `keys()` and `values()` functions for JSON objects.
@markborkum markborkum mentioned this pull request Mar 7, 2019
3 tasks
@dmlb2000
Copy link
Member

dmlb2000 commented Mar 8, 2019

@markborkum I did update the version of pylint on you, since we are supporting Python 3.x only. That means we can push pylint beyond 1.x now.

@markborkum
Copy link
Contributor Author

@dmlb2000 Sounds good. I'm working on 100% coverage at the moment. Then I'll circle back to the pylint "duplicate-code" warnings.

@dmlb2000
Copy link
Member

dmlb2000 commented Mar 8, 2019

@markborkum There was an interesting duplicate key in hash warning from flake8 as well did you see that?

@markborkum
Copy link
Contributor Author

@dmlb2000 That's resolved by ea0325b. Now we're just left with the "duplicate-code" warnings.

@markborkum
Copy link
Contributor Author

@dmlb2000 I think that we should pause work on this branch.

There are two issues:

  1. pylint "duplicate-code" warnings; and
  2. Coverage of instance methods in ECMAScript standard.

The first issue is what it is, but we need to decide the second issue. At the moment, the coverage is very low.

@dmlb2000
Copy link
Member

dmlb2000 commented Mar 8, 2019

@markborkum What other functions would need to be implemented to cover the ECMAScript standard?

@markborkum
Copy link
Contributor Author

markborkum commented Mar 8, 2019

@dmlb2000 The following should be covered:

  • Properties, e.g., length for JSON arrays and strings.
  • Class and instance methods, where: (a) the class is a JSON value; (b) all arguments are JSON values; (c) the return value is a JSON value; and (d) the instance is not mutated.

Array:

  • length
  • concat
  • entries
  • fill
  • flat
  • includes
  • indexOf
  • join
  • keys
  • lastIndexOf
  • slice
  • sort
  • splice
  • values

JSON:

  • parse
  • stringify

Math:

  • abs
  • acos
  • acosh
  • asin
  • asinh
  • atan
  • atan2
  • atanh
  • cbrt
  • ceil
  • clz32
  • cos
  • cosh
  • exp
  • expm1
  • floor
  • fround
  • hypot
  • imul
  • log
  • log10
  • log1p
  • log2
  • max
  • min
  • pow
  • random
  • round
  • sign
  • sin
  • sinh
  • sqrt
  • tan
  • tanh
  • trunc

Number:

  • isFinite
  • isInteger
  • isNaN
  • isSafeInteger
  • parseFloat
  • parseInt

Object:

  • entries
  • keys
  • values

String:

  • charAt
  • codeCharAt
  • codePointAt
  • concat
  • endsWith
  • includes
  • indexOf
  • lastIndexOf
  • length
  • localeCompare
  • match
  • normalize
  • padEnd
  • padStart
  • repeat
  • replace
  • search
  • slice
  • split
  • startsWith
  • substring
  • toLocaleLowerCase
  • toLocaleUpperCase
  • toLowerCase
  • toUpperCase
  • trim
  • trimEnd
  • trimStart

@dmlb2000
Copy link
Member

dmlb2000 commented Mar 8, 2019

@markborkum This is a lot, we'll leave this open for folks to contribute too and hopefully we can get some community support for this.

Thanks for your effort!

@markborkum
Copy link
Contributor Author

@khm Your thoughts about coverage?

@khm
Copy link

khm commented Mar 11, 2019

@khm Your thoughts about coverage?

I think it would be worth raising an unsupported exception of some kind instead of requiring full coverage as a release condition. That way we can accept community contributions as need arises. If I'm answering the wrong question, let me know.

dmlb2000 and others added 2 commits April 8, 2019 10:53
Signed-off-by: David Brown <[email protected]>

Conflicts:
	jsonpath2/expressions/operator.py
	jsonpath2/node.py
	jsonpath2/path.py
	jsonpath2/subscript.py
@dmlb2000
Copy link
Member

@markborkum This is ready to merge?

@dmlb2000
Copy link
Member

So to be clear for others coming into this bug...

We are going to merge this in, even though the pull request is technically incomplete. This will provide folks with working functions in a jsonpath. The future work is to be explicit about what functions we support and what functions are not completed. We will have future effort to make it easy for folks to add new functions to extend the jsonpath spec. Example pull requests will be referenced here for folks to have a path forward to extend jsonpath.

@dmlb2000 dmlb2000 merged commit 29d53e8 into pacifica:master May 17, 2019
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.

3 participants