Update cuds api according to m30 by mehdisadeghi · Pull Request #312 · simphony/simphony-common

mehdisadeghi · 2016-08-02T16:05:27Z

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.

The main parameter to add/get/remove items changed to `name`. Some `uid` specific methods added to carry out the same tasks with `uid`. Code simplified. `name` and `description` added to Simulation and CUDS classes. Perhaps I have to create multiple commits next time.

codecov-io · 2016-08-02T16:10:32Z

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 · 2016-08-02T16:17:25Z

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

kitchoi · 2016-08-02T16:21:12Z

+                 description=None):
+        self.name = name
+        self.description = description
+


Simulation does not have __slots__. User can always do:

sim = Simulation(cuds, engine_name) sim.name = 'sim1' sim.description = 'description for sim1'

Or, are the names/descriptions required in the API (then they should not be default to be None)?

They are optional at the moment, exactly like the meta classes. Perhaps we have to turn CUDS and Simulation classes into CUDSItem derivaties, to have one universal data structure to deal with (it has been always the aim). However, for now I just decided to keep it simple.

I agree with @mehdisadeghi here. For now it should be enough.

kitchoi · 2016-08-02T16:38:23Z

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?

tuopuu · 2016-08-03T06:32:00Z

-                self.data = {}
-        self.dummpy_component1 = DummyComponent()
-        self.dummpy_component2 = DummyComponent()
+        self.dummpy_component1 = api.Box(name='mybox')


While at it, should fix this to self.dummy_component1 & 2. :)

Oh gotcha! I'll fix them right away! :)

mehdisadeghi · 2016-08-03T15:00:57Z

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 · 2016-08-04T00:03:45Z

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 · 2016-08-04T14:53:57Z

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

kitchoi · 2016-08-04T17:06:44Z

+        return self._uid
+
+    @uid.setter
+    def uid(self, value):


If you don't provide the setter, it is read-only. Trying to set it would raise an AttributeError: can't set attribute

I agree, please remove the setter and deleter readonly properties only need the property decorated function only

Good to know that. I'll remove setters and deleters.

kitchoi · 2016-08-04T17:24:34Z

Is there a test_get_nameless_component_by_name version of test_remove_nameless_component_by_name?

kitchoi · 2016-08-04T20:33:50Z

+            if the component is not a CUDS component
        ValueError
-            if the component is already added
+            if a component already exists


if a component with the same name already exists?

Yes, corrected.

kitchoi · 2016-08-04T20:54:13Z

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 · 2016-08-04T20:55:38Z

+
+        self.assertEqual(c.get_by_uid(self.nameless_cuds_1.uid),
+                         self.nameless_cuds_1)
+        self.assertIsNone(c.get(self.nameless_cuds_1.name))


I guess my comment in CUDS.get is about whether it is a better user-experience to raise an Error here if name is None or empty.

kitchoi · 2016-08-04T21:02:39Z

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.

itziakos · 2016-08-05T11:44:41Z

    @property
    def data(self):
        """Data container for CUDS items."""
        return DataContainer(self._data)


What is the reasoning for creating a (shallow) copy of the self._data. If the purpose is to have a snapshot then this will work only if the contained keys and values are immutable. In any other case we have an object that references internal information that can change without anybody knowing about them.

If I am correct only the engine functions and returned dataset are required to support the snapshot principle. Does this extend to simulation? what is there a reason for this?

You are correct, this will not work properly with our current mutable CUDS objects. I think the behavior of the data property regarding meta classes is unclear.

I think Simulation can have the same API as CUDS.

Mehdi Sadeghi added 4 commits July 28, 2016 15:00

Replace uuid with uid. Fix #307

0e62963

Merge branch 'master' into update-cuds-api-according-to-m30

2dd7501

Update cuds tests using generated meta.

70e74e6

kitchoi reviewed Aug 2, 2016
View reviewed changes

tuopuu reviewed Aug 3, 2016
View reviewed changes

Fix the funny typo

474b61d

Mehdi Sadeghi added 4 commits August 4, 2016 11:52

Accept items with no name.

3a04f5a

Add uid to CUDS

1d092e7

Add more tests.

ee2e8e0

Remove misleading comment and useless check.

bc58c3c

kitchoi reviewed Aug 4, 2016
View reviewed changes

itziakos reviewed Aug 5, 2016
View reviewed changes

Remove useless setters and deleters

c29dad5

Uh oh!

Conversation

mehdisadeghi commented Aug 2, 2016

Uh oh!

codecov-io commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 72.40% (diff: 91.30%)

Uh oh!

mehdisadeghi commented Aug 2, 2016

Uh oh!

kitchoi Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kitchoi commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehdisadeghi commented Aug 3, 2016

Uh oh!

ahashibon commented Aug 4, 2016

Uh oh!

mehdisadeghi commented Aug 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kitchoi commented Aug 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kitchoi commented Aug 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kitchoi commented Aug 4, 2016

Uh oh!

itziakos Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Aug 2, 2016 •

edited

Loading

kitchoi Aug 2, 2016 •

edited

Loading

kitchoi commented Aug 2, 2016 •

edited

Loading

itziakos Aug 5, 2016 •

edited

Loading