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

json.get no longer interprets string/number as an array of size 1 #1144

Closed
Merudo opened this issue Jan 17, 2020 · 12 comments
Closed

json.get no longer interprets string/number as an array of size 1 #1144

Merudo opened this issue Jan 17, 2020 · 12 comments
Assignees
Labels
bug macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.

Comments

@Merudo
Copy link
Member

Merudo commented Jan 17, 2020

Describe the bug
The function json.get no longer implicitly converts strings/numbers to arrays of size 1.

Previously, json.get(string, 0) would return the string, and json.get(number, 0) would return the number. Now it gives an error instead.

To Reproduce
Steps to reproduce the behavior:

  1. Type [r: json.get("test", 0)]
  2. In 1.5.11 alpha 7, it returns the error Argument number 1 to function "json.get" must be a JSON Array or Object.
  3. In 1.5.10, it instead returns test.

Expected behavior
json.get works like it did in 1.5.10.

MapTool Info

  • Version: 1.5.11 alpha 7, develop (1/17/2020)
  • Install: New

Desktop:

  • OS: Windows
  • Version: 10

Additional context
The new behavior may be preferable to the previous one. If we keep it, the change logs should document the new behavior of json.get.

@Phergus Phergus added bug macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) labels Jan 17, 2020
@Azhrei
Copy link
Member

Azhrei commented Jan 17, 2020

I’m inclined to go with the 1.5.10 functionality as a bug. Anyone else want to chime in?

@Phergus
Copy link
Contributor

Phergus commented Jan 17, 2020

That was my first thought as well but I bet changing the behavior will break frameworks.

@Azhrei
Copy link
Member

Azhrei commented Jan 18, 2020

I’m only about 20% worried about that.

If a FW breaks, they were relying on undocumented implementation artifacts. (We didn’t document it to work like that, I hope?) And I doubt anyone would write code like that and actually expect that result!

@melek
Copy link
Collaborator

melek commented Jan 18, 2020

If it could break frameworks, maybe the change should be punted to 1.6 in the interest of semantic versioning? It seems reasonable if it would technically be breaking backward-compatibility a tiny bit, even if it isn't intended behavior.

I'm not attached to it going in now or later, but I think the idea is worth mentioning.

@aliasmask
Copy link

There are several functions that treat "string" as an array of 1 item.

[r: json.difference(json.append("","one","two"),"one")]

@aliasmask
Copy link

It may be bad coding, but that's our audience.

@Merudo
Copy link
Member Author

Merudo commented Jan 18, 2020

If a FW breaks, they were relying on undocumented implementation artifacts. (We didn’t document it to work like that, I hope?) And I doubt anyone would write code like that and actually expect that result!

In my case, the issue occurred because of json.path.read. By default, json.path.read returns a json array if there are more than one element, but it can return a string if there is a single element.

Hence the following code results in an error in 1.5.11 (but not 1.5.10) if json.path.read returns a single element:

[element = json.path.read(array, condition)]
[value = json.get(element, 0)]

Thankfully, now that json.path.read can take a config parameter with ALWAYS_RETURN_LIST, it won't be an issue anymore.

@Azhrei
Copy link
Member

Azhrei commented Jan 18, 2020

Well, waiting for 1.6 isn’t terrible either, since that’s likely just around the corner.

But I don’t any skin in this game, so while I have an opinion, it’s all that I have! I’ll defer to the people that write MTscript. Since @aliasmask says there are others that work the same way, I’m inclined to leave it (otherwise, we should change all of them).

@Phergus
Copy link
Contributor

Phergus commented Jan 18, 2020

I don't really see any harm in continuing with the way it has worked in the past.

@cwisniew cwisniew self-assigned this Jan 18, 2020
@cwisniew
Copy link
Member

I will fix it to preserve compatibility

@aliasmask
Copy link

I've recently been caught being lazy with json.contains where if you use "" for your empty array it will toss back an error. For example, if I get a token property where it either doesn't exist or has a json array I have to do some validation to change to "[]" if empty so my code doesn't break later on.

Unknown JSON type "" in function "json.contains".

@Phergus
Copy link
Contributor

Phergus commented Jan 18, 2020

Tested. Working as before.

@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Jan 18, 2020
Phergus added a commit that referenced this issue Jan 21, 2020
@Phergus Phergus closed this as completed Jan 23, 2020
Merudo added a commit to Merudo/maptool that referenced this issue Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.
Projects
None yet
Development

No branches or pull requests

6 participants