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.

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

fix verdi computer show tab completion #4962

Merged
merged 2 commits into from
Jun 2, 2021

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented May 26, 2021

Got the following error when doing verdi computer show <tab>. This is a quick fix but just thinking when this change made in DbComputer, maybe other places also need to be take care of?

Traceback (most recent call last):                                                                                      
  File "/home/unkcpz/Projects/python-code/aiida_core/aiida/orm/implementation/querybuilder.py", line 399, in get_column 
    return getattr(alias, colname)                                                                                      
  File "/home/unkcpz/miniconda3/envs/aiida-core-dev/lib/python3.8/site-packages/sqlalchemy/orm/util.py", line 506, in __
getattr__                                                                                                               
    attr = getattr(target, key)                                                                                         
AttributeError: type object 'DbComputer' has no attribute 'name'
...
...
  File "/home/unkcpz/Projects/python-code/aiida_core/aiida/orm/querybuilder.py", line 1225, in _add_to_projections
    entity_to_project = self._get_projectable_entity(alias, column_name, attr_key, cast=cast)
  File "/home/unkcpz/Projects/python-code/aiida_core/aiida/orm/querybuilder.py", line 1199, in _get_projectable_entity
    entity = self._impl.get_column(column_name, alias)
  File "/home/unkcpz/Projects/python-code/aiida_core/aiida/orm/implementation/querybuilder.py", line 401, in get_column
    raise ValueError(
ValueError: name is not a column of aliased(DbComputer)
Valid columns are:
id
uuid
label
hostname
description
scheduler_type
transport_type
metadata

@sphuber sphuber self-requested a review May 26, 2021 09:30
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @unkcpz . The change of DbComputer.name to DbComputer.label was done in #4882 . It should have updated all other places in the code that still used name but this one was clearly missed and it is not tested. It would be great if you could add a quick test for it. Note that I think therefore we should also project on label instead of the hostname, because that is essentially what was being projected originally.

aiida/cmdline/params/types/computer.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #4962 (d46a75a) into develop (3214236) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4962      +/-   ##
===========================================
+ Coverage    80.05%   80.06%   +0.01%     
===========================================
  Files          515      515              
  Lines        36611    36611              
===========================================
+ Hits         29307    29308       +1     
+ Misses        7304     7303       -1     
Flag Coverage Δ
django 74.52% <100.00%> (+0.02%) ⬆️
sqlalchemy 73.44% <100.00%> (-<0.01%) ⬇️

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

Impacted Files Coverage Δ
aiida/cmdline/params/types/computer.py 81.82% <100.00%> (+2.28%) ⬆️
aiida/engine/daemon/client.py 75.13% <0.00%> (-1.01%) ⬇️
aiida/transports/plugins/local.py 81.80% <0.00%> (+0.26%) ⬆️
aiida/orm/utils/loaders.py 86.52% <0.00%> (+0.47%) ⬆️

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 3214236...d46a75a. Read the comment docs.

@unkcpz unkcpz force-pushed the fix/computel-compl-hostname branch from 55e319b to 145302c Compare May 26, 2021 10:40
@unkcpz
Copy link
Member Author

unkcpz commented May 26, 2021

Thanks @sphuber. The test added and also refactoring the unit test to use pytest.

@unkcpz
Copy link
Member Author

unkcpz commented May 26, 2021

Well, seems run into pre-commit pylint warnings. Do we need to exclude tests/ in pre-commit-pylint?

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the test and refactoring @unkcpz . The tests should also be linted so it should not be added to the exclude list. The errors are easy to address though. I made three suggestions that show how to do it. Just the one of unused variables you should apply to the other locations in the code that pylint mentioned

@@ -8,88 +8,110 @@
# For further information please visit http://www.aiida.net #
###########################################################################
"""Tests for the `ComputerParamType`."""
Copy link
Contributor

Choose a reason for hiding this comment

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

pylint: disable=redefined-outer-name



@pytest.fixture
def setup_computers(clear_database_before_test):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def setup_computers(clear_database_before_test):
@pytest.mark.usefixtures('clear_database_before_test')
def setup_computers():

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah~oh, mark.usefixtures does not work on fixtures ;-) pytest-dev/pytest#977

"""
Verify that using the ID will retrieve the correct entity
"""
entity_01, entity_02, entity_03 = setup_computers
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
entity_01, entity_02, entity_03 = setup_computers
entity_01, _, _ = setup_computers

@unkcpz unkcpz force-pushed the fix/computel-compl-hostname branch from 145302c to 2198872 Compare May 26, 2021 11:50
@unkcpz
Copy link
Member Author

unkcpz commented May 26, 2021

@sphuber thanks! It should be all right this time.

@unkcpz unkcpz force-pushed the fix/computel-compl-hostname branch from 2198872 to 16943c4 Compare May 26, 2021 12:09
@unkcpz unkcpz force-pushed the fix/computel-compl-hostname branch from 16943c4 to d6638b7 Compare May 26, 2021 12:15
@sphuber sphuber merged commit 3489d05 into aiidateam:develop Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants