CLI: verdi profile setdefault to set-default #6426

agoscinski · 2024-05-28T14:01:47Z

Since verdi user uses set-default, but verdi profile uses setdefault, we make the two consistent by using set-default for profile. Old command is marked as deprecated till version 3

sphuber

Thanks @agoscinski some minor requests

sphuber · 2024-05-28T20:57:23Z

src/aiida/cmdline/utils/decorators.py

+        message_ = message
+        if version is not None:
+            message_ = f'{message_} (this will be removed in v{version})'


Why the temporary reassignment, wouldn't the following be identical?

Suggested change

message_ = message

if version is not None:

message_ = f'{message_} (this will be removed in v{version})'

if version is not None:

message = f'{message} (this will be removed in v{version})'

This will fail. As soon as we want to assign message it is assumed to be local, so the message variable that comes from outside is now not accessible, but in the assignment we need it. So one way is to make a new local variable or to mark message as global, but then we change it outside of the function which might be confusing if the code ever gets extended.

A simple example of this is behavior.

x = 1 def foo(): x = x return x

This will cause a UnboundLocalError: cannot access local variable 'x' where it is not associated with a value. While this works.

x = 1 def foo(): global x x = x return x

But I agree the code seems weird when one is not aware of this, and it is unintuitive pythonic behavior. I changed the logic a bit to avoid thinking about this.

This will fail.

Are you sure?
The following example seems to suggest otherwise:

In [1]: def deprecated_command(message, version=None): ...: if version is not None: ...: message = f'{message} (this will be removed in v{version})' ...: print(message) ...: In [2]: deprecated_command('This is a message') This is a message In [3]: deprecated_command('This is a message', version=3) This is a message (this will be removed in v3)

Am I missing anything?

Yes the reason why your example works is because the message is given as input argument of the function. The local function structure in the decorator function is important for this to fail, because message needs to be accessed out of the scope of the function.

Ah yeah, I see now. Then it makes sense. Maybe the following change makes it a bit cleaner still:

message = wrap(message if version is None else message + f' (this will be removed in v{version})') echo.echo(template.render(msg=message, width - 4), width=width)

I used a different name that is more meaningful to bypass the problem. Let me know what you think. I don't have any strong feelings about this, I just did this already before your comment, it was just not pushed.

I also added use packaging.version.Version object to standardize the version always to the X.Y.Z format. I follow python's internal logic for versioning Version('1.0') == Version('1.0.0') -> True. Not sure if this is following some more general versioning specification but at least it is standardized within Python.

I am actually not sure If should keep this change in this PR, as I don't use it anymore. It could be useful, but I also can store this changes locally and in a PR on my fork.

I could implement this version logic to in VerdiCommand that requires a new initialization parameter deprecated_version. I can also put this as an issue so we can get this faster merged and implement the version later as marking a version for 3.0.0 as deprecated is not so urgent.

I just realized what you meant that this is in the deprecated_command decorator which is more or less "deprecated" itself, as it is just automatically used by the VerdiCommand. I agree with you that this doesn't really belong in this PR. Let's just take it out for now and then we can follow up with this later. Probably we need to add an argument to the command method to declare this version if we think it is useful. And it can then pass this on to echo_deprecated.

src/aiida/cmdline/commands/cmd_user.py

sphuber · 2024-05-28T21:00:17Z

src/aiida/cmdline/commands/cmd_profile.py

 @arguments.PROFILE(required=True, default=None)
 def profile_setdefault(profile):
-    """Set a profile as the default one."""
+    """Set a profile as the default profile."""


Maybe we add the fact that it is deprecated manually such that it shows up in the help text when we do verdi profile --help?

Suggested change

"""Set a profile as the default profile."""

"""Set a profile as the default profile. (deprecated)"""

But I don't think we do this for other commands, so maybe it is fine to skip this and keep it as is.

I think I would use the same method we will use in #6433 once we decided there the way to go

Agreed, let's wrap that one up first, then you can use that method here as well

I now have the same pattern as in verdi setup

tests/cmdline/commands/test_profile.py

sphuber · 2024-06-04T07:32:05Z

@agoscinski can we wrap this one up for the release?

src/aiida/cmdline/utils/decorators.py

codecov · 2024-06-06T14:02:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.87%. Comparing base (ef60b66) to head (1ab92bf).
Report is 112 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6426      +/-   ##
==========================================
+ Coverage   77.51%   77.87%   +0.37%     
==========================================
  Files         560      562       +2     
  Lines       41444    41808     +364     
==========================================
+ Hits        32120    32555     +435     
+ Misses       9324     9253      -71

Flag	Coverage Δ
presto	`73.20% <62.50%> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sphuber

All good @agoscinski . Just two open suggestions

src/aiida/cmdline/utils/decorators.py

agoscinski · 2024-06-06T15:00:25Z

Okay rebased, tests should pass now. Separated this PR into two commits, since the versioning has become a separated feature.

sphuber · 2024-06-06T15:04:11Z

Okay rebased, tests should pass now. Separated this PR into two commits, since the versioning has become a separated feature.

Thanks @agoscinski but I would prefer to get rid of the first commit. With your previous PR, the deprecated_command decorator is no longer needed. I just opened a PR to deprecate it and update all deprecated commands to use your new method: #6461

So it doesn't make sense to add this new feature to something we are deprecating and no longer using.

Since verdi user uses set-default, we make the two commands consistent by using set-default for profile and deprecating setdefault. Fix for aiidateam#2910.

agoscinski · 2024-06-06T15:07:23Z

Sure! Makes sense. Just dropped it

sphuber · 2024-06-06T15:38:04Z

Thanks a lot @agoscinski . As an aside, it is not necessary to add the PR number in the commit message title. This is done automatically when merging through github. Also, we don't really add issue numbers to commit messages. We just add them to the OP of the PR instead. That should be sufficient

…aiidateam#6426) This is for consistency with `verdi user set-default`.

agoscinski marked this pull request as ready for review May 28, 2024 14:35

sphuber requested changes May 28, 2024

View reviewed changes

sphuber reviewed Jun 5, 2024

View reviewed changes

src/aiida/cmdline/utils/decorators.py Outdated Show resolved Hide resolved

agoscinski force-pushed the profile-setdefault-to-set-default branch from fba1216 to 7605bb5 Compare June 6, 2024 13:41

agoscinski requested a review from sphuber June 6, 2024 13:42

sphuber requested changes Jun 6, 2024

View reviewed changes

src/aiida/cmdline/utils/decorators.py Outdated Show resolved Hide resolved

agoscinski force-pushed the profile-setdefault-to-set-default branch from e42c5d1 to 72dc8de Compare June 6, 2024 14:59

CLI: Change verdi profile setdefault to set-default (aiidateam#6426)

1ab92bf

Since verdi user uses set-default, we make the two commands consistent by using set-default for profile and deprecating setdefault. Fix for aiidateam#2910.

agoscinski force-pushed the profile-setdefault-to-set-default branch from 72dc8de to 1ab92bf Compare June 6, 2024 15:06

This was referenced Jun 6, 2024

Profile setdefault to set default backup #6462

Closed

Implement optional version argument into VerdiCommand that specifies when a deprecated command is removed #6463

Open

agoscinski requested a review from sphuber June 6, 2024 15:35

sphuber merged commit ab48a4f into aiidateam:main Jun 6, 2024
10 of 12 checks passed

mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024

CLI: Rename verdi profile setdefault to verdi profile set-default (…

edefc4a

…aiidateam#6426) This is for consistency with `verdi user set-default`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: verdi profile setdefault to set-default #6426

CLI: verdi profile setdefault to set-default #6426

agoscinski commented May 28, 2024 •

edited by sphuber

Loading

sphuber left a comment

sphuber May 28, 2024

agoscinski Jun 5, 2024 •

edited

Loading

sphuber Jun 5, 2024

agoscinski Jun 5, 2024

sphuber Jun 5, 2024

agoscinski Jun 6, 2024

agoscinski Jun 6, 2024

agoscinski Jun 6, 2024

sphuber Jun 6, 2024

sphuber May 28, 2024

sphuber May 28, 2024

agoscinski Jun 5, 2024

sphuber Jun 5, 2024

agoscinski Jun 6, 2024

sphuber commented Jun 4, 2024

codecov bot commented Jun 6, 2024 •

edited

Loading

sphuber left a comment

agoscinski commented Jun 6, 2024

sphuber commented Jun 6, 2024

agoscinski commented Jun 6, 2024

sphuber commented Jun 6, 2024

	"""Set a profile as the default profile."""
	"""Set a profile as the default profile. (deprecated)"""

CLI: verdi profile setdefault to set-default #6426

CLI: verdi profile setdefault to set-default #6426

Conversation

agoscinski commented May 28, 2024 • edited by sphuber Loading

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski Jun 5, 2024 • edited Loading

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

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

sphuber commented Jun 4, 2024

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

sphuber left a comment

Choose a reason for hiding this comment

agoscinski commented Jun 6, 2024

sphuber commented Jun 6, 2024

agoscinski commented Jun 6, 2024

sphuber commented Jun 6, 2024

agoscinski commented May 28, 2024 •

edited by sphuber

Loading

agoscinski Jun 5, 2024 •

edited

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading