simphony / simphony-common

The native implementation of the Simphony cuds objects
BSD 2-Clause "Simplified" License
7 stars 5 forks source link

Update cuds api according to m30 #312

Closed mehdisadeghi closed 8 years ago

mehdisadeghi commented 8 years ago

This PR simplifies CUDS class. Adds name and description to CUDS and Simulation classes. Moreover, uses name as the main parameter to add/get/remove items inside CUDS.

codecov-io commented 8 years ago

Current coverage is 72.40% (diff: 91.30%)

Merging #312 into master will increase coverage by 0.88%

@@             master       #312   diff @@
==========================================
  Files           115        115          
  Lines          6949       6969    +20   
  Methods           0          0          
  Messages          0          0          
  Branches        803        808     +5   
==========================================
+ Hits           4970       5046    +76   
+ Misses         1952       1880    -72   
- Partials         27         43    +16   

Powered by Codecov. Last update 4d4a56c...2731f2a

mehdisadeghi commented 8 years ago

@tuopuu @kitchoi @SGGgarcia Please review this PR. @ahashibon Please see the changes regarding name parameter.

kitchoi commented 8 years ago

Looks like with this PR, it is then necessary to define a unique string name for every item being added to a CUDS. name essentially replaces uuid. Is this right? If so, is this behaviour intended by the SSB?

mehdisadeghi commented 8 years ago

name essentially replaces uuid. Is this right?

To some extent, yes. uid for internal consistency, name for users.

is this behaviour intended by the SSB

AFAIK, Yes. Hence mentioning Adham to confirm.

As I have commented in the code, the behavior of add/get/remove by name vs. by uid is ambiguous. What happens if a user adds a Box and Material with the exact same names? The initial behavior was to replace them blindly (as in python dict), I forced it to reject duplicates to have further discussions.

ahashibon commented 8 years ago

As I have commented in the code, the behavior of add/get/remove by name vs. by uid is ambiguous. What happens if a user adds a Box and Material with the exact same names? The initial behavior was to replace them blindly (as in python dict), I forced it to reject duplicates to have further discussions. For now this is ok. Next iteration we should allow the same name for different items. For example a Box and Material may have the same name, but two different materials cannot. In essence this would require a way in the API to distinguish between the items.

mehdisadeghi commented 8 years ago

@tuopuu @kitchoi @SGGgarcia based on our discussions I made some changes. Please have a look.

kitchoi commented 8 years ago

Is there a test_get_nameless_component_by_name version of test_remove_nameless_component_by_name?

kitchoi commented 8 years ago

Please ignore the comment on the existence of test_get_nameless_component_by_name, i saw the test that i was looking for that is named differently.

kitchoi commented 8 years ago

Thanks @mehdisadeghi! Tests are diligent. There are a few minor issues above, except for that, the rest looks good to merge to me. Side note: I remain puzzled by the existence of CUDS.data, but I think that's a separate issue/question.