👌 IMPROVE: constructor of base data types #5165

mbercx · 2021-10-05T14:30:07Z

Adapt the constructor of the Dict and List data types so the
value no longer needs to be provided as a keyword argument, but can
simply be passed as the first positional argument.

Remove the *args from the BaseType constructor, instead specifying
the value input argument. Otherwise users could just pass multiple
positional arguments and it would only use the first without raising an
error.

Also fixes two issues with the List class:

The remove method was not working as prescribed. Instead of
removing the first element in the list with the specified value, it
simply deleted the element with index value.
The set_list() method didn't make a copy of the list input, so any
modification done to the List instance would also affect the original
list. If the same list is used to initialise several List instances,
adapting one would affect the other.

Finally, add tests for the List class.

mbercx · 2021-10-06T19:31:07Z

Seems there were already some tests for List in tests/test_base_dataclasses.py. Will check why this one fails and move them to tests/orm/data/test_list.py.

I also noticed that these tests use test classes based on AiidaTestCase. Is there a preference between using this setup and separate functions decorated with @pytest.mark.usefixtures('clear_database_before_test')?

chrisjsewell · 2021-10-06T19:33:21Z

I also noticed that these tests use test classes based on AiidaTestCase. Is there a preference between using this setup and separate functions decorated with @pytest.mark.usefixtures('clear_database_before_test')?

yep AiidaTestCase is "deprecated", see #4782 for converting

mbercx · 2021-10-06T21:34:12Z

Alright, refactored the basetype tests. I also moved the data dir in the tests to nodes (renamed from node, to match the package directory tree.

Some things I found out while working on this:

Although adding together a str and bool is perfectly possible:

In [1]: 'a' + 'b'
Out[1]: 'ab'

In [2]: True + False
Out[2]: 1

Doing this for Str or Bool raises a TypeError: unsupported operand type(s).

abs is not implemented for the BaseType classes:

In [3]: abs(Float(-2.0))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-324814f1c59f> in <module>
----> 1 abs(Float(-2.0))

TypeError: bad operand type for abs(): 'Float'

Some interesting inplace addition behaviour:

In [4]: f = Float(2)

In [4]: f += 2.0

In [5]: f
Out[5]: <Float: uuid: 8ee7db38-6a28-442f-8bfb-827de8f9d897 (unstored) value: 4.0>

In [6]: f.store()
Out[6]: <Float: uuid: 8ee7db38-6a28-442f-8bfb-827de8f9d897 (pk: 1) value: 4.0>

In [7]: f += 0.1

In [8]: f
Out[8]: <Float: uuid: cbf4b5ee-1474-46d6-aa48-c11a0639d12f (unstored) value: 4.1>

codecov · 2021-10-06T21:56:21Z

Codecov Report

Merging #5165 (63e590d) into develop (08ac107) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #5165      +/-   ##
===========================================
+ Coverage    81.31%   81.31%   +0.01%     
===========================================
  Files          529      529              
  Lines        37031    37031              
===========================================
+ Hits         30107    30108       +1     
+ Misses        6924     6923       -1

Flag	Coverage Δ
django	`76.81% <100.00%> (+0.01%)`	⬆️
sqlalchemy	`75.76% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/orm/nodes/data/base.py	`83.34% <100.00%> (-1.96%)`	⬇️
aiida/orm/nodes/data/dict.py	`87.24% <100.00%> (ø)`
aiida/orm/nodes/data/list.py	`91.02% <100.00%> (+6.31%)`	⬆️
aiida/orm/nodes/data/bool.py	`94.12% <0.00%> (-5.88%)`	⬇️
aiida/engine/daemon/client.py	`75.26% <0.00%> (-1.01%)`	⬇️
aiida/transports/plugins/local.py	`81.41% <0.00%> (-0.25%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08ac107...63e590d. Read the comment docs.

unkcpz

Thanks, @mbercx! Looks good to me.

As for the tests files move to node/data/, I like this new folder refactoring, it has more parallel structure with the code they test.

unkcpz · 2021-12-02T14:59:01Z

aiida/orm/nodes/data/dict.py

+    def __init__(self, value=None, **kwargs):
+        """Initialise a ``Dict`` node instance.


Just want to make sure this will not break any backwards compatibility, correct? We can still use d = orm.Dict(dict={}).

Okay, I see a test case for this particular. 👍

Adapt the constructor of the `Dict` and `List` data types so the value no longer needs to be provided as a keyword argument, but can simply be passed as the first input. Remove the `*args` from the `BaseType` constructor, instead specifying the `value` input argument. Otherwise users could just pass multiple positional arguments and it would only use the first without raising an error. Also fixes two issues with the `List` class: * The `remove` method was not working as prescribed. Instead of removing the first element in the list with the specified value, it simply deleted the element with index `value`. * The `set_list()` method didn't make a copy of the `list` input, so any modification done to the `List` instance would also affect the original `list`. If the same list is used to initialise several `List` instances, adapting one would affect the other. Finally, add tests for the `List` class.

mbercx · 2021-12-06T12:52:32Z

Thanks for the review @unkcpz! Just rebased, good to merge once the test complete.

unkcpz

Thanks @mbercx!

We recently adapted the constructors of the `Dict` and `List` nodes so they no longer require the `dict` and `list` keywords, see: aiidateam/aiida-core#5165 Here we add a transformer that automatically removes these keywords from `Dict` and `List` constructors if present.

mbercx requested review from chrisjsewell and ramirezfranciscof October 5, 2021 14:30

mbercx mentioned this pull request Oct 5, 2021

Small changes to List and Dict data types API #4495

Closed

mbercx force-pushed the fix/4495/dict-list-constructor branch from 11b421e to 82fc7aa Compare October 6, 2021 01:02

mbercx force-pushed the fix/4495/dict-list-constructor branch from 82fc7aa to eb830de Compare October 6, 2021 21:31

mbercx force-pushed the fix/4495/dict-list-constructor branch 2 times, most recently from 4b32d62 to 499c4fe Compare October 20, 2021 09:21

ramirezfranciscof requested a review from unkcpz November 22, 2021 09:04

ramirezfranciscof assigned unkcpz Nov 22, 2021

unkcpz previously approved these changes Dec 2, 2021

View reviewed changes

mbercx added 2 commits December 6, 2021 13:35

Refactor data tests

63e590d

mbercx dismissed unkcpz’s stale review via 63e590d December 6, 2021 12:36

mbercx force-pushed the fix/4495/dict-list-constructor branch from 499c4fe to 63e590d Compare December 6, 2021 12:36

unkcpz enabled auto-merge (squash) December 6, 2021 13:56

unkcpz approved these changes Dec 6, 2021

View reviewed changes

unkcpz merged commit eb73c7d into aiidateam:develop Dec 6, 2021

mbercx deleted the fix/4495/dict-list-constructor branch December 6, 2021 14:56

mbercx mentioned this pull request Mar 10, 2022

✨ NEW: Transformer for removing dict and list keywords aiidateam/aiida-upgrade#4

Merged

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👌 IMPROVE: constructor of base data types #5165

👌 IMPROVE: constructor of base data types #5165

mbercx commented Oct 5, 2021 •

edited

Loading

mbercx commented Oct 6, 2021

chrisjsewell commented Oct 6, 2021

mbercx commented Oct 6, 2021

codecov bot commented Oct 6, 2021 •

edited

Loading

unkcpz left a comment

unkcpz Dec 2, 2021

unkcpz Dec 2, 2021

mbercx commented Dec 6, 2021

unkcpz left a comment

		def __init__(self, value=None, **kwargs):
		"""Initialise a ``Dict`` node instance.

👌 IMPROVE: constructor of base data types #5165

👌 IMPROVE: constructor of base data types #5165

Conversation

mbercx commented Oct 5, 2021 • edited Loading

mbercx commented Oct 6, 2021

chrisjsewell commented Oct 6, 2021

mbercx commented Oct 6, 2021

codecov bot commented Oct 6, 2021 • edited Loading

Codecov Report

unkcpz left a comment

Choose a reason for hiding this comment

unkcpz Dec 2, 2021

Choose a reason for hiding this comment

unkcpz Dec 2, 2021

Choose a reason for hiding this comment

mbercx commented Dec 6, 2021

unkcpz left a comment

Choose a reason for hiding this comment

mbercx commented Oct 5, 2021 •

edited

Loading

codecov bot commented Oct 6, 2021 •

edited

Loading