Skip to content

Conversation

ambs
Copy link
Contributor

@ambs ambs commented Apr 1, 2019

Hello again!

I found a mistake I added in the last PR, calling _attr as a method. Probably I should rewrite it as a method and adapt the code. If you wish I can do that. But for now, fixed the mistake. Sorry!

I was adding a new functionality on my module (btw, its Arango::DB) and I needed a flag whose valid values are 0, 1 or 2. So, I added a small functionality to allow user-defined types (well, leaf types).

Finally, I am giving priority to builtin adjusting methods. But this approach can be used to allow user-defined methods, just changing the order by which adjust methods are searched. Just not sure we should allow if for array and dictionary/hash.

Thanks!

@@ -29,9 +29,19 @@ my @tests = (
{aa=>0.1, bb=>0.1},
{aa=>0}
],
[ { type => 'fruit' }, "apple", "apple" ],
Copy link
Owner

Choose a reason for hiding this comment

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

Such usage of 'type' breaks json-schema specification (it allows just closed list of types).
Maybe it's better to rely on 'format' value?
{ type => 'string', format => 'fruit' }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. I confess I tried lookup for the Json-Schema specification, but found different document. We are basing on this specification? https://json-schema.org/

If so, I will read it and try to understand what you are suggesting. Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

@ambs
Copy link
Contributor Author

ambs commented Apr 2, 2019

Was looking to the enum type. The main problem of implementing it is detecting the type of each element of the array. I would suggest:

  • the use of JSON::true and JSON::false directly for booleans
  • calling encode_json on the remaining items, and check if they are similar. (using eq)

While not 100%, this approach would allow part of the intended behaviour.

Thoughts?

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.

2 participants