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

Change 'legalities' field to an array #66

Closed
Sembiance opened this Issue Aug 3, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@Sembiance
Collaborator

Sembiance commented Aug 3, 2015

The 'legalities' field right now is an object like this:

{    "Legacy" : "Banned",
    "Vintage" : "Restricted",
  "Commander" : "Legal" }

It should probably be changed to an array like this:

[ {   "format" : "Legacy",
    "legality" : "Banned" },
  {   "format" : "Vintage",
    "legality" : "Restricted" },
  {   "format" : "Commander",
    "legality" : "Legal" } ]

This would make it in line with how the 'rulings' and 'foreignNames' fields work. Additionally having predictable key names would make mapping the JSON to programming language objects/classes much easier.

This of course would break any apps/websites using this field, which is why I haven't done it yet.

@Sembiance Sembiance added the discuss label Aug 3, 2015

@jenjia

This comment has been minimized.

Show comment
Hide comment
@jenjia

jenjia Aug 3, 2015

In my opinion, It is fine as it is, as the time passes, more info is added to MTGJson, and you should keep the files as short as possible.
I go further, and say that the others field that you talk should approach this style.

jenjia commented Aug 3, 2015

In my opinion, It is fine as it is, as the time passes, more info is added to MTGJson, and you should keep the files as short as possible.
I go further, and say that the others field that you talk should approach this style.

@Sembiance

This comment has been minimized.

Show comment
Hide comment
@Sembiance

Sembiance Aug 4, 2015

Collaborator

@jenjia I actually agree with you. I find it much easier to read the way it is, and it's also smaller. If it were just my own project I would also change languages to be the same way { "German" : "Sumpf", etc. }

However, I had two users report difficulties importing the data when 'dynamic keys' were involved. One mentioned that using defined key names in an array would be a more consistent with the data model where keys are keys and values are values.

This issue is marked as 'discuss' as I'm open to other opinions. I'm glad to hear that you like it the more concise way as I like it that way too, but I feel like it's a bit like arguing how many spaces a tab should be :)

Collaborator

Sembiance commented Aug 4, 2015

@jenjia I actually agree with you. I find it much easier to read the way it is, and it's also smaller. If it were just my own project I would also change languages to be the same way { "German" : "Sumpf", etc. }

However, I had two users report difficulties importing the data when 'dynamic keys' were involved. One mentioned that using defined key names in an array would be a more consistent with the data model where keys are keys and values are values.

This issue is marked as 'discuss' as I'm open to other opinions. I'm glad to hear that you like it the more concise way as I like it that way too, but I feel like it's a bit like arguing how many spaces a tab should be :)

@jenjia

This comment has been minimized.

Show comment
Hide comment
@jenjia

jenjia Aug 4, 2015

In this specific example (Legacy, Vintage and Commander) we are talking an increase of 1.7MB (+/-), it doesn't seems much. But then when we talk about the parse time, in mobile apps, for me it already takes ~5 seconds to parse AllSets-x.json every time the app is launched, I would love to reduce this time to its limit :)

jenjia commented Aug 4, 2015

In this specific example (Legacy, Vintage and Commander) we are talking an increase of 1.7MB (+/-), it doesn't seems much. But then when we talk about the parse time, in mobile apps, for me it already takes ~5 seconds to parse AllSets-x.json every time the app is launched, I would love to reduce this time to its limit :)

@flyingmutant

This comment has been minimized.

Show comment
Hide comment
@flyingmutant

flyingmutant Aug 4, 2015

While you are right in that decreasing the size of JSON is an important thing, for really quick startup you'd probably better use something like FlatBuffers instead, to avoid parsing altogether. Startup with parsing 50MB of JSON will never be too snappy.

flyingmutant commented Aug 4, 2015

While you are right in that decreasing the size of JSON is an important thing, for really quick startup you'd probably better use something like FlatBuffers instead, to avoid parsing altogether. Startup with parsing 50MB of JSON will never be too snappy.

@Sembiance

This comment has been minimized.

Show comment
Hide comment
@Sembiance

Sembiance Aug 17, 2015

Collaborator

So I ran into an issue today where I was super glad that the 'foreignNames' field was an array of objects and not an object itself. I wanted to add an additional field per foreignName and that wouldn't have been possible if it was an object like this 'legalities' field is. So if I were to add new information to the legalities information in the future, say a comment or a 'startingFrom' date or something like that, the current format wouldn't work.

Thus I have decided to proceed with this change.

Collaborator

Sembiance commented Aug 17, 2015

So I ran into an issue today where I was super glad that the 'foreignNames' field was an array of objects and not an object itself. I wanted to add an additional field per foreignName and that wouldn't have been possible if it was an object like this 'legalities' field is. So if I were to add new information to the legalities information in the future, say a comment or a 'startingFrom' date or something like that, the current format wouldn't work.

Thus I have decided to proceed with this change.

@Sembiance Sembiance closed this in 6ee2c9c Aug 18, 2015

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