validate label string at code setup stage #3793

unkcpz · 2020-02-24T08:48:37Z

Prevent users from using '@' when naming the code when calling verdi code setup.
Not just make this validation in verdi code relabel

ltalirz

thanks for this fix @unkcpz !

I have just one request, namely to overwrite the setter instead of introducing a new function:

aiida-core/aiida/orm/nodes/node.py

Lines 221 to 227 in 5e26bac

    
               @label.setter 
        
               def label(self, value): 
        
                   """Set the label. 
        
                   :param value: the new value to set 
        
                   """ 
        
                   self.backend_entity.label = value

Your setter should first do the validation and then call the setter of the superclass.

unkcpz · 2020-02-24T15:45:36Z

It should be right now. yeah!

unkcpz · 2020-02-24T15:48:18Z

And I search the whole package without finding raise_error being set, so I remove this parameter. Hope it won't be a problem.

ltalirz

thanks @unkcpz , some final minor requests

aiida/orm/nodes/data/code.py

ltalirz · 2020-02-24T16:02:06Z

aiida/orm/nodes/data/code.py

@@ -131,21 +152,14 @@ def relabel(self, new_label, raise_error=True):
        if new_label.endswith(suffix):
            new_label = new_label[:-len(suffix)]

-        if '@' in new_label:
-            msg = "Code labels must not contain the '@' symbol"
-            if raise_error:


Ok to deprecate it - in this case please put in the docstring of the function something like

.. deprecated:: 1.2.0 Will be removed in `v2.0.0`. Use `try/except` instead.

for the raise_error argument

I'll remove the argument, it will make label setter complex otherwise.

What you've done here is fine (leave the argument for the moment but don't do anything with it).
Can you still put the deprecation warning about the raise_error argument into the docstring of relabel?

aiida/orm/nodes/data/code.py

aiida/orm/utils/builders/code.py

ltalirz

looks good, just one thing left to do

ltalirz · 2020-02-25T07:15:58Z

aiida/orm/nodes/data/code.py

@@ -131,21 +152,14 @@ def relabel(self, new_label, raise_error=True):
        if new_label.endswith(suffix):
            new_label = new_label[:-len(suffix)]

-        if '@' in new_label:
-            msg = "Code labels must not contain the '@' symbol"
-            if raise_error:


What you've done here is fine (leave the argument for the moment but don't do anything with it).
Can you still put the deprecation warning about the raise_error argument into the docstring of relabel?

add deprecate info for raise_error in function relabel

unkcpz · 2020-02-25T12:04:04Z

done ;-) thanks for reviewing @ltalirz

ltalirz

Thanks @unkcpz , that seems good to go.

One thing I just noticed - there seems to be no test of this either...
Would be good to add one, but I'm ok to merge without it as well.

unkcpz · 2020-02-25T12:12:19Z

the label validate is test in here

aiida-core/tests/cmdline/commands/test_code.py

Line 186 in 9fcb78a

def test_relabel_code_full_bad(self):

it call the label setter, so it sort of being tested I think.

ltalirz · 2020-02-25T12:14:12Z

Right - it wasn't tested before but now it is. Great!

unkcpz · 2020-02-25T12:17:27Z

In fact the validation only works when the whole code config is passed in. which means if user type @ in the label interactively will see the exception at the end of config stage.
But I think that needs to be sorted out separately.

sphuber

Thanks @unkcpz . I unfortunately missed the review window, but I have one comment and a question.

sphuber · 2020-02-25T15:20:57Z

aiida/orm/nodes/data/code.py

+        """
+        if '@' in str(value):
+            msg = "Code labels must not contain the '@' symbol"
+            raise InputValidationError(msg)


This should have been just a ValuError. The InputValidationError is intended for use in CalcJob.prepare_for_submission

Sorry I didn't see this - this code is just copy-pasted from where it was before.
There are a lot of places in aiida-core where this error is used that have nothing to do with prepare_for_submission:

aiida-core/aiida/orm/querybuilder.py

Line 141 in 09cb6cb

raise InputValidationError('I do not know what to do with {}'.format(cls))

Do all of these uses have to be replaced?

I would say so yes, but that would be a breaking change. The InputValidationError is not a subclass of ValueError which is what one would expect for these kinds of errors. I will open an issue to address this for 2.0

sphuber · 2020-02-25T15:21:34Z

aiida/orm/nodes/data/code.py

        self.label = new_label

    def get_description(self):
        """Return a string description of this Code instance.

        :return: string description of this Code instance
+
+        .. deprecated:: 1.2.0


Why is this deprecated? Almost all of the ORM classes have get_description. If we are going for a property, we should try to be consistent and do this everywhere.

I would deprecate this for all classes but didn't want to ask jason to do this in this PR.
Sounds reasonable?

I am not sure if we really should replace get_description for description. The reason that both exist is that description is understood to return the value stored in the description column and get_description is some compound from various columns/attributes. We might be able to find another name for get_description to reduce confusion, but in any case I think it should be done in a consistent way across the code and not just in one place

Ok, I'll revert this deprecation at some point.

ltalirz suggested changes Feb 24, 2020

View reviewed changes

unkcpz force-pushed the label-validate branch from db7889a to 498ef65 Compare February 24, 2020 15:43

ltalirz suggested changes Feb 24, 2020

View reviewed changes

unkcpz force-pushed the label-validate branch from 498ef65 to c505bc2 Compare February 25, 2020 01:11

ltalirz reviewed Feb 25, 2020

View reviewed changes

validate label string at code setup stage

7462187

add deprecate info for raise_error in function relabel

unkcpz force-pushed the label-validate branch from c505bc2 to 7462187 Compare February 25, 2020 12:01

ltalirz approved these changes Feb 25, 2020

View reviewed changes

Merge branch 'develop' into label-validate

7a388e7

ltalirz merged commit 67d4504 into aiidateam:develop Feb 25, 2020

sphuber reviewed Feb 25, 2020

View reviewed changes

giovannipizzi mentioned this pull request Mar 2, 2020

Remove wrong deprecation warning for get_description #3818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate label string at code setup stage #3793

validate label string at code setup stage #3793

unkcpz commented Feb 24, 2020

ltalirz left a comment

unkcpz commented Feb 24, 2020

unkcpz commented Feb 24, 2020

ltalirz left a comment

ltalirz Feb 24, 2020 •

edited

Loading

unkcpz Feb 25, 2020

ltalirz Feb 25, 2020

ltalirz left a comment

ltalirz Feb 25, 2020

unkcpz commented Feb 25, 2020

ltalirz left a comment

unkcpz commented Feb 25, 2020

ltalirz commented Feb 25, 2020

unkcpz commented Feb 25, 2020

sphuber left a comment

sphuber Feb 25, 2020

ltalirz Feb 25, 2020

sphuber Feb 27, 2020

sphuber Feb 25, 2020

ltalirz Feb 25, 2020

sphuber Feb 27, 2020

ltalirz Feb 27, 2020

	@label.setter
	def label(self, value):
	"""Set the label.

	:param value: the new value to set
	"""
	self.backend_entity.label = value

validate label string at code setup stage #3793

validate label string at code setup stage #3793

Conversation

unkcpz commented Feb 24, 2020

ltalirz left a comment

Choose a reason for hiding this comment

unkcpz commented Feb 24, 2020

unkcpz commented Feb 24, 2020

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Feb 25, 2020

ltalirz left a comment

Choose a reason for hiding this comment

unkcpz commented Feb 25, 2020

ltalirz commented Feb 25, 2020

unkcpz commented Feb 25, 2020

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Feb 24, 2020 •

edited

Loading