simphony / simphony-common

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

Feature: Create new uuid when add component to CUDS #288

Closed kitchoi closed 8 years ago

kitchoi commented 8 years ago

Consistent with Particles and Mesh, new uuid can be created when a component to be added does not already have one (e.g. component.uid is None).

Alternatively, we can make sure every CUDS component has a uuid created upon __init__, but it does not seem to me necessary.

Note that I have removed converting uuid to str as uuid is already hashable (a pretty good hash) so we don't need to convert it to str to be hashed again.

The type checking for the component_id in the add, get and remove functions wasn't symmetric (i.e. it is possible to add something you can't remove later, see FIXME comment). Here I removed these type checking altogether, but we might want to add them back.

codecov-io commented 8 years ago

Current coverage is 96.51%

Merging #288 into master will increase coverage by +0.09%

@@           master       #288   diff @@
========================================
  Files          47         47          
  Lines        3807       3806     -1   
  Methods         0          0          
  Branches      573        571     -2   
========================================
+ Hits         3670       3673     +3   
+ Misses         46         44     -2   
+ Partials       91         89     -2   
  1. File ...ols/lattice_tools.py (not in diff) was modified. more
    • Misses -1
    • Partials -1
    • Hits +2

Sunburst

Powered by Codecov. Last updated by 35e552e

mehdisadeghi commented 8 years ago

@kitchoi You have removed the answer to the life universe and everything from the tests!

kitchoi commented 8 years ago

@mehdisadeghi ahaha yes, because cuds.get(42) gives you None, and I consider this tested already.

mehdisadeghi commented 8 years ago

@kitchoi does this PR needs a corresponding issue?

kitchoi commented 8 years ago

@mehdisadeghi Could do.

mehdisadeghi commented 8 years ago

@kitchoi nevermind. Just merged.