SimplifiedLogic / creoson

OpenSource Automation using JSON Transactions for PTC's CREO Parametric
http://www.creoson.com
MIT License
81 stars 23 forks source link

`material` features do not returns expected data #53

Closed Zepmanbc closed 3 years ago

Zepmanbc commented 3 years ago

Hi,

Got an e-mail from Samuel, few weeks ago, who found some unexpected things about materials features. Here is my analysis:

file : list_materials / list_materials_wildcard

The request:

{'sessionId': '8811647924416275374', 'command': 'file', 'function': 'list_materials', 'data': {'file': 'myfile.prt'}}

When material is found:

{'status': {'error': False}, 'data': {'materials': ['MY_MATERIAL']}}

When there is no material:

{'status': {'error': False}}

Expected when no material found:

{'status': {'error': False}, 'data': {'materials': []}

file : get_cur_material / get_cur_material_wildcard

The request:

{'sessionId': '-892181493656888120', 'command': 'file', 'function': 'get_cur_material', 'data': {'file': 'c2v5.prt'}}

When material is found:

{'status': {'error': False}, 'data': {'material': 'MY_MATERIAL'}}

When there is no material

{'status': {'error': True, 'message': 'Error handling request: Unexpected null value'}}

Expected when no material:

{'status': {'error': False}, 'data': {'material': null}

file : set_cur_material

The request:

{'sessionId': '143602516292714635', 'command': 'file', 'function': 'set_cur_material', 'data': {'material': 'MY_MATERIAL', 'file': 'my_file.prt'}}

When material has been set:

{'status': {'error': False}}

Expected (from the doc)

{'status': {'error': False}, 'data': {'files': ['my_file.prt']}}

When material has not been loadded (this is fine, it's just to be exhausive) :

{'status': {'error': True, 'message': 'Material MY_MATERIAL has not been loaded on file: my_file.prt'}}

gazontimide commented 3 years ago

Adding to this, same thing happens with the list_symbols function in a drawing. If there is no symbols in the drawing, I get an error stating missing data in creoson return.

Is this intended as this way? Just like the other functions up here, I would simply expect a response with empty data, not an exception.

Thanks

adama2000 commented 3 years ago

In general, list functions are set up to return nothing when there is no data. Should we change that? Would changing it break anyone who's expecting the current behavior?

I'll make a fix for the get_cur_material functions.

adama2000 commented 3 years ago

Couldn't duplicate your error with get_cur_material. Could the error be occurring in the creopyson layer because we're not returning a data object?

adama2000 commented 3 years ago

Should we make a global change to creoson to always return a "data" object even if the object is empty?

Zepmanbc commented 3 years ago

Couldn't duplicate your error with get_cur_material. Could the error be occurring in the creopyson layer because we're not returning a data object?

The "expected" from the doc? ({'status': {'error': False}, 'data': {'files': ['my_file.prt']}})

Should we make a global change to creoson to always return a "data" object even if the object is empty?

I don't really get all the consequences but i like the idea of the result is the expected type, if it's a list, even if it's empty,it's a list

adama2000 commented 3 years ago

That's the "expected" for set_cur_material, I was talking about your get_cur_material example.

As for the list returns, I guess I've always disliked wasted memory, and an empty list is wasted memory.

gazontimide commented 3 years ago

I am simply an amateur as far as programming goes, so I don't think I could properly answer adama2000's question about whether to always return a data object, even if empty, especially since i've never really had to deal with memory usage! My answer would then be a little bit biased haha.

On another note, I've started using the family table list function and it always returns a list. If there are no instances, it'll return an empty list, like I would personnally expect from such a command. So maybe there's just a little bit of inconsistencies? I don't know if it's in the python module or Creoson's.

adama2000 commented 3 years ago

That's a fair point about the family table function.

adama2000 commented 3 years ago

@Zepmanbc still hoping for an answer on the get_cur_material question.

Zepmanbc commented 3 years ago

Couldn't duplicate your error with get_cur_material. Could the error be occurring in the creopyson layer because we're not returning a data object?

creopyson is waiting for data in the result but the error is True in the response, so it is from Creoson:

{'status': {'error': True, 'message': 'Error handling request: Unexpected null value'}}

in the documentation file-get_cur_material : Example 2 it should return no data and error False

request:

{"sessionId": "~sessionId\~","command": "file","function": "get_cur_material","data": {"file": "wingnut.prt"}}

response:

{"status": {"error": false}}

I can send you my empty file for testing, but it's really an empty file... I've done this tests on Creo4. Or maybe send me the wingnut.prt file and i'll try if i get the same response as the documentation.


I don't really manage memory stuff, my answer for empty list is more a point of view for the abstraction.


Sorry for the delay of response, lots of things to do.

Zepmanbc commented 3 years ago

Here is the request/response for familytable/list to illustrate gazontimide's message

request

{'command': 'familytable', 'data': {'file': 'myfile.prt'}, 'function': 'list', 'sessionId': '1667098025249847893'}

response:

{"status":{"error":false},"data":{"instances":[]}}

adama2000 commented 3 years ago

Okay I've identified the problem with the get_cur_material code. I'll make a fix for it. Sorry!

adama2000 commented 3 years ago

I took a survey of our "list" functions. When there are no results, 52% of them return an empty list and 48% of them return nothing.

This seems inconsistent, but I'm not sure which way it should be changed to behave.

adama2000 commented 3 years ago

Dang, I finally realized the set_cur_material return is a bug too. I'll fix that.