Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate label string at code setup stage #3793

Merged
merged 2 commits into from
Feb 25, 2020
Merged

validate label string at code setup stage #3793

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7462187
validate label string at code setup stage
unkcpz Feb 24, 2020
7a388e7
Merge branch 'develop' into label-validate
ltalirz Feb 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion aiida/cmdline/commands/cmd_code.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,11 @@ def setup_code(non_interactive, **kwargs):
kwargs['code_type'] = CodeBuilder.CodeType.STORE_AND_UPLOAD

code_builder = CodeBuilder(**kwargs)
code = code_builder.new()

try:
code = code_builder.new()
except InputValidationError as exception:
echo.echo_critical('invalid inputs: {}'.format(exception))

try:
code.store()
Expand Down
35 changes: 27 additions & 8 deletions aiida/orm/nodes/data/code.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,32 +120,51 @@ def full_label(self):
"""
return '{}@{}'.format(self.label, self.get_computer_name())

@property
unkcpz marked this conversation as resolved.
Show resolved Hide resolved
def label(self):
"""Return the node label.

:return: the label
"""
return super().label

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

:param value: the new value to set
"""
if '@' in str(value):
msg = "Code labels must not contain the '@' symbol"
raise InputValidationError(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


super(Code, self.__class__).label.fset(self, value)

def relabel(self, new_label, raise_error=True):
"""Relabel this code.

:param new_label: new code label
:param raise_error: Set to False in order to return a list of errors
instead of raising them.

.. deprecated:: 1.2.0
Will remove raise_error in `v2.0.0`. Use `try/except` instead.
"""
suffix = '@{}'.format(self.get_computer_name())
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:
Copy link
Member

@ltalirz ltalirz Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

raise InputValidationError(msg)
else:
return InputValidationError(msg)

self.label = new_label

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

unkcpz marked this conversation as resolved.
Show resolved Hide resolved
:return: string description of this Code instance

.. deprecated:: 1.2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Will be removed in `v2.0.0`. Use `.description` property instead
"""
return '{}'.format(self.label)
return '{}'.format(self.description)

@classmethod
def get_code_helper(cls, label, machinename=None):
Expand Down