Skip to content
This repository was archived by the owner on Apr 23, 2021. It is now read-only.

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

Merged
mehdisadeghi merged 2 commits into
masterfrom
feature/cuds-add-create-uuid-2
Apr 26, 2016
Merged

Feature: Create new uuid when add component to CUDS#288
mehdisadeghi merged 2 commits into
masterfrom
feature/cuds-add-create-uuid-2

Conversation

@kitchoi

@kitchoi kitchoi commented Apr 25, 2016

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

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

@kitchoi

kitchoi commented Apr 26, 2016

Copy link
Copy Markdown
Contributor Author

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

@mehdisadeghi

Copy link
Copy Markdown
Contributor

@kitchoi does this PR needs a corresponding issue?

@kitchoi

kitchoi commented Apr 26, 2016

Copy link
Copy Markdown
Contributor Author

@mehdisadeghi Could do.

@mehdisadeghi mehdisadeghi merged commit bf0fd84 into master Apr 26, 2016
@mehdisadeghi

Copy link
Copy Markdown
Contributor

@kitchoi nevermind. Just merged.

@mehdisadeghi mehdisadeghi deleted the feature/cuds-add-create-uuid-2 branch April 26, 2016 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants